From dbf6215490a55e41258fb8abc040ab1e7a18d033 Mon Sep 17 00:00:00 2001 From: zhilingc Date: Sun, 5 Apr 2020 10:16:11 +0800 Subject: [PATCH 1/2] Clean up OnlineServingService code to be more readable --- .../serving/service/OnlineServingService.java | 61 ++++++++++--------- .../main/java/feast/serving/util/Metrics.java | 36 +++++++++-- .../main/java/feast/serving/util/RefUtil.java | 8 +++ .../api/retrieval/FeatureSetRequest.java | 7 +++ 4 files changed, 76 insertions(+), 36 deletions(-) diff --git a/serving/src/main/java/feast/serving/service/OnlineServingService.java b/serving/src/main/java/feast/serving/service/OnlineServingService.java index c6f2178a7f2..11ef6683196 100644 --- a/serving/src/main/java/feast/serving/service/OnlineServingService.java +++ b/serving/src/main/java/feast/serving/service/OnlineServingService.java @@ -16,16 +16,13 @@ */ package feast.serving.service; -import static feast.serving.util.Metrics.requestCount; -import static feast.serving.util.Metrics.staleKeyCount; -import static feast.serving.util.RefUtil.generateFeatureStringRef; - import com.google.common.collect.Maps; import com.google.protobuf.Duration; import feast.serving.ServingAPIProto.*; import feast.serving.ServingAPIProto.GetOnlineFeaturesRequest.EntityRow; import feast.serving.ServingAPIProto.GetOnlineFeaturesResponse.FieldValues; import feast.serving.specs.CachedSpecService; +import feast.serving.util.Metrics; import feast.serving.util.RefUtil; import feast.storage.api.retrieval.FeatureSetRequest; import feast.storage.api.retrieval.OnlineRetriever; @@ -65,7 +62,7 @@ public GetFeastServingInfoResponse getFeastServingInfo( /** {@inheritDoc} */ @Override public GetOnlineFeaturesResponse getOnlineFeatures(GetOnlineFeaturesRequest request) { - try (Scope scope = tracer.buildSpan("Redis-getOnlineFeatures").startActive(true)) { + try (Scope scope = tracer.buildSpan("getOnlineFeatures").startActive(true)) { GetOnlineFeaturesResponse.Builder getOnlineFeaturesResponseBuilder = GetOnlineFeaturesResponse.newBuilder(); List featureSetRequests = @@ -85,13 +82,11 @@ public GetOnlineFeaturesResponse getOnlineFeatures(GetOnlineFeaturesRequest requ List featureRowsForFs = featureRows.get(fsIdx); FeatureSetRequest featureSetRequest = featureSetRequests.get(fsIdx); + String project = featureSetRequest.getSpec().getProject(); + // In order to return values containing the same feature references provided by the user, // we reuse the feature references in the request as the keys in the featureValuesMap - Map featureNames = - featureSetRequest.getFeatureReferences().stream() - .collect( - Collectors.toMap( - FeatureReference::getName, featureReference -> featureReference)); + Map refsByName = featureSetRequest.getFeatureRefsByName(); // Each feature row returned (per feature set request) corresponds to a given entity row. // For each feature row, update the featureValuesMap. @@ -106,36 +101,23 @@ public GetOnlineFeaturesResponse getOnlineFeatures(GetOnlineFeaturesRequest requ .parallelStream() .forEach( ref -> { - staleKeyCount - .labels( - featureSetRequest.getSpec().getProject(), - String.format("%s:%d", ref.getName(), ref.getVersion())) - .inc(); + populateStaleKeyCountMetrics(project, ref); featureValuesMap .get(entityRow) .put(RefUtil.generateFeatureStringRef(ref), Value.newBuilder().build()); }); } else { - featureSetRequest - .getFeatureReferences() - .parallelStream() - .forEach( - ref -> - requestCount - .labels( - featureSetRequest.getSpec().getProject(), - String.format("%s:%d", ref.getName(), ref.getVersion())) - .inc()); + populateRequestCountMetrics(featureSetRequest); // Else populate the featureValueMap at this entityRow with the values in the feature // row. featureRow.getFieldsList().stream() - .filter(field -> featureNames.containsKey(field.getName())) + .filter(field -> refsByName.containsKey(field.getName())) .forEach( field -> { - FeatureReference ref = featureNames.get(field.getName()); - String id = generateFeatureStringRef(ref); + FeatureReference ref = refsByName.get(field.getName()); + String id = RefUtil.generateFeatureStringRef(ref); featureValuesMap.get(entityRow).put(id, field.getValue()); }); } @@ -150,6 +132,24 @@ public GetOnlineFeaturesResponse getOnlineFeatures(GetOnlineFeaturesRequest requ } } + private void populateStaleKeyCountMetrics(String project, FeatureReference ref) { + Metrics.staleKeyCount() + .labels(project, RefUtil.generateFeatureStringRefWithoutProject(ref)) + .inc(); + } + + private void populateRequestCountMetrics(FeatureSetRequest featureSetRequest) { + String project = featureSetRequest.getSpec().getProject(); + featureSetRequest + .getFeatureReferences() + .parallelStream() + .forEach( + ref -> + Metrics.requestCount() + .labels(project, RefUtil.generateFeatureStringRefWithoutProject(ref)) + .inc()); + } + @Override public GetBatchFeaturesResponse getBatchFeatures(GetBatchFeaturesRequest getFeaturesRequest) { throw Status.UNIMPLEMENTED.withDescription("Method not implemented").asRuntimeException(); @@ -162,7 +162,8 @@ public GetJobResponse getJob(GetJobRequest getJobRequest) { private boolean isStale( FeatureSetRequest featureSetRequest, EntityRow entityRow, FeatureRow featureRow) { - if (featureSetRequest.getSpec().getMaxAge().equals(Duration.getDefaultInstance())) { + Duration maxAge = featureSetRequest.getSpec().getMaxAge(); + if (maxAge.equals(Duration.getDefaultInstance())) { return false; } long givenTimestamp = entityRow.getEntityTimestamp().getSeconds(); @@ -170,6 +171,6 @@ private boolean isStale( givenTimestamp = System.currentTimeMillis() / 1000; } long timeDifference = givenTimestamp - featureRow.getEventTimestamp().getSeconds(); - return timeDifference > featureSetRequest.getSpec().getMaxAge().getSeconds(); + return timeDifference > maxAge.getSeconds(); } } diff --git a/serving/src/main/java/feast/serving/util/Metrics.java b/serving/src/main/java/feast/serving/util/Metrics.java index fa66f79a804..37a6f31f980 100644 --- a/serving/src/main/java/feast/serving/util/Metrics.java +++ b/serving/src/main/java/feast/serving/util/Metrics.java @@ -21,7 +21,7 @@ public class Metrics { - public static final Histogram requestLatency = + private static final Histogram requestLatency = Histogram.build() .buckets(0.001, 0.002, 0.004, 0.006, 0.008, 0.01, 0.015, 0.02, 0.025, 0.03, 0.035, 0.05) .name("request_latency_seconds") @@ -30,7 +30,7 @@ public class Metrics { .labelNames("method") .register(); - public static final Counter requestCount = + private static final Counter requestCount = Counter.build() .name("request_feature_count") .subsystem("feast_serving") @@ -38,7 +38,7 @@ public class Metrics { .labelNames("project", "feature_name") .register(); - public static final Counter missingKeyCount = + private static final Counter missingKeyCount = Counter.build() .name("missing_feature_count") .subsystem("feast_serving") @@ -46,7 +46,7 @@ public class Metrics { .labelNames("project", "feature_name") .register(); - public static final Counter invalidEncodingCount = + private static final Counter invalidEncodingCount = Counter.build() .name("invalid_encoding_feature_count") .subsystem("feast_serving") @@ -54,7 +54,7 @@ public class Metrics { .labelNames("project", "feature_name") .register(); - public static final Counter staleKeyCount = + private static final Counter staleKeyCount = Counter.build() .name("stale_feature_count") .subsystem("feast_serving") @@ -62,11 +62,35 @@ public class Metrics { .labelNames("project", "feature_name") .register(); - public static final Counter grpcRequestCount = + private static final Counter grpcRequestCount = Counter.build() .name("grpc_request_count") .subsystem("feast_serving") .help("number of grpc requests served") .labelNames("method", "status_code") .register(); + + public static Histogram requestLatency() { + return requestLatency; + } + + public static Counter requestCount() { + return requestCount; + } + + public static Counter missingKeyCount() { + return missingKeyCount; + } + + public static Counter invalidEncodingCount() { + return invalidEncodingCount; + } + + public static Counter staleKeyCount() { + return staleKeyCount; + } + + public static Counter grpcRequestCount() { + return grpcRequestCount; + } } diff --git a/serving/src/main/java/feast/serving/util/RefUtil.java b/serving/src/main/java/feast/serving/util/RefUtil.java index 74de3e65620..c3bcb0827a2 100644 --- a/serving/src/main/java/feast/serving/util/RefUtil.java +++ b/serving/src/main/java/feast/serving/util/RefUtil.java @@ -28,6 +28,14 @@ public static String generateFeatureStringRef(FeatureReference featureReference) return ref; } + public static String generateFeatureStringRefWithoutProject(FeatureReference featureReference) { + String ref = String.format("%s", featureReference.getName()); + if (featureReference.getVersion() > 0) { + return ref + String.format(":%d", featureReference.getVersion()); + } + return ref; + } + public static String generateFeatureSetStringRef(FeatureSetSpec featureSetSpec) { String ref = String.format("%s/%s", featureSetSpec.getProject(), featureSetSpec.getName()); if (featureSetSpec.getVersion() > 0) { diff --git a/storage/api/src/main/java/feast/storage/api/retrieval/FeatureSetRequest.java b/storage/api/src/main/java/feast/storage/api/retrieval/FeatureSetRequest.java index 27a80a8f4aa..bd3a8bc6912 100644 --- a/storage/api/src/main/java/feast/storage/api/retrieval/FeatureSetRequest.java +++ b/storage/api/src/main/java/feast/storage/api/retrieval/FeatureSetRequest.java @@ -21,6 +21,8 @@ import feast.core.FeatureSetProto.FeatureSetSpec; import feast.serving.ServingAPIProto.FeatureReference; import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; @AutoValue public abstract class FeatureSetRequest { @@ -50,4 +52,9 @@ public Builder addFeatureReference(FeatureReference featureReference) { public abstract FeatureSetRequest build(); } + + public Map getFeatureRefsByName() { + return getFeatureReferences().stream() + .collect(Collectors.toMap(FeatureReference::getName, featureReference -> featureReference)); + } } From 18a1b4e82bfabc69d82d7936ba78bc807ef5530b Mon Sep 17 00:00:00 2001 From: zhilingc Date: Sun, 5 Apr 2020 10:24:21 +0800 Subject: [PATCH 2/2] Revert Metrics --- .../serving/service/OnlineServingService.java | 4 +-- .../main/java/feast/serving/util/Metrics.java | 36 ++++--------------- 2 files changed, 8 insertions(+), 32 deletions(-) diff --git a/serving/src/main/java/feast/serving/service/OnlineServingService.java b/serving/src/main/java/feast/serving/service/OnlineServingService.java index 11ef6683196..3a91a3c955d 100644 --- a/serving/src/main/java/feast/serving/service/OnlineServingService.java +++ b/serving/src/main/java/feast/serving/service/OnlineServingService.java @@ -133,7 +133,7 @@ public GetOnlineFeaturesResponse getOnlineFeatures(GetOnlineFeaturesRequest requ } private void populateStaleKeyCountMetrics(String project, FeatureReference ref) { - Metrics.staleKeyCount() + Metrics.staleKeyCount .labels(project, RefUtil.generateFeatureStringRefWithoutProject(ref)) .inc(); } @@ -145,7 +145,7 @@ private void populateRequestCountMetrics(FeatureSetRequest featureSetRequest) { .parallelStream() .forEach( ref -> - Metrics.requestCount() + Metrics.requestCount .labels(project, RefUtil.generateFeatureStringRefWithoutProject(ref)) .inc()); } diff --git a/serving/src/main/java/feast/serving/util/Metrics.java b/serving/src/main/java/feast/serving/util/Metrics.java index 37a6f31f980..fa66f79a804 100644 --- a/serving/src/main/java/feast/serving/util/Metrics.java +++ b/serving/src/main/java/feast/serving/util/Metrics.java @@ -21,7 +21,7 @@ public class Metrics { - private static final Histogram requestLatency = + public static final Histogram requestLatency = Histogram.build() .buckets(0.001, 0.002, 0.004, 0.006, 0.008, 0.01, 0.015, 0.02, 0.025, 0.03, 0.035, 0.05) .name("request_latency_seconds") @@ -30,7 +30,7 @@ public class Metrics { .labelNames("method") .register(); - private static final Counter requestCount = + public static final Counter requestCount = Counter.build() .name("request_feature_count") .subsystem("feast_serving") @@ -38,7 +38,7 @@ public class Metrics { .labelNames("project", "feature_name") .register(); - private static final Counter missingKeyCount = + public static final Counter missingKeyCount = Counter.build() .name("missing_feature_count") .subsystem("feast_serving") @@ -46,7 +46,7 @@ public class Metrics { .labelNames("project", "feature_name") .register(); - private static final Counter invalidEncodingCount = + public static final Counter invalidEncodingCount = Counter.build() .name("invalid_encoding_feature_count") .subsystem("feast_serving") @@ -54,7 +54,7 @@ public class Metrics { .labelNames("project", "feature_name") .register(); - private static final Counter staleKeyCount = + public static final Counter staleKeyCount = Counter.build() .name("stale_feature_count") .subsystem("feast_serving") @@ -62,35 +62,11 @@ public class Metrics { .labelNames("project", "feature_name") .register(); - private static final Counter grpcRequestCount = + public static final Counter grpcRequestCount = Counter.build() .name("grpc_request_count") .subsystem("feast_serving") .help("number of grpc requests served") .labelNames("method", "status_code") .register(); - - public static Histogram requestLatency() { - return requestLatency; - } - - public static Counter requestCount() { - return requestCount; - } - - public static Counter missingKeyCount() { - return missingKeyCount; - } - - public static Counter invalidEncodingCount() { - return invalidEncodingCount; - } - - public static Counter staleKeyCount() { - return staleKeyCount; - } - - public static Counter grpcRequestCount() { - return grpcRequestCount; - } }