From c0834f3eaf4b17f46ee6729f5b813312bb58a719 Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Thu, 13 Oct 2022 07:44:54 -0700 Subject: [PATCH 1/8] updated observability config to public preview config --- .../gcp/observability/GcpObservability.java | 6 +- .../observability/ObservabilityConfig.java | 55 ++- .../ObservabilityConfigImpl.java | 207 ++++++---- .../interceptors/ConfigFilterHelper.java | 172 ++------- .../InternalLoggingChannelInterceptor.java | 204 +++++----- .../InternalLoggingServerInterceptor.java | 204 +++++----- .../gcp/observability/logging/GcpLogSink.java | 19 +- .../grpc/gcp/observability/LoggingTest.java | 80 +--- .../grpc/gcp/observability/MetricsTest.java | 2 +- .../ObservabilityConfigImplTest.java | 360 ++++++++++++++---- .../io/grpc/gcp/observability/TracesTest.java | 2 +- .../interceptors/ConfigFilterHelperTest.java | 211 ++++------ ...InternalLoggingChannelInterceptorTest.java | 117 +----- .../InternalLoggingServerInterceptorTest.java | 96 +---- .../observability/logging/GcpLogSinkTest.java | 6 +- 15 files changed, 787 insertions(+), 954 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java index e993242d80e..770d764e0cd 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java @@ -76,15 +76,15 @@ public static synchronized GcpObservability grpcInit() throws IOException { if (instance == null) { GlobalLocationTags globalLocationTags = new GlobalLocationTags(); ObservabilityConfigImpl observabilityConfig = ObservabilityConfigImpl.getInstance(); - Sink sink = new GcpLogSink(observabilityConfig.getDestinationProjectId(), + Sink sink = new GcpLogSink(observabilityConfig.getProjectId(), globalLocationTags.getLocationTags(), observabilityConfig.getCustomTags(), SERVICES_TO_EXCLUDE); LogHelper helper = new LogHelper(sink); - ConfigFilterHelper configFilterHelper = ConfigFilterHelper.factory(observabilityConfig); + ConfigFilterHelper configFilterHelper = ConfigFilterHelper.getInstance(observabilityConfig); instance = grpcInit(sink, observabilityConfig, new InternalLoggingChannelInterceptor.FactoryImpl(helper, configFilterHelper), new InternalLoggingServerInterceptor.FactoryImpl(helper, configFilterHelper)); - instance.registerStackDriverExporter(observabilityConfig.getDestinationProjectId(), + instance.registerStackDriverExporter(observabilityConfig.getProjectId(), observabilityConfig.getCustomTags()); } return instance; diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java index d4da16637d2..fc44988ff84 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java @@ -17,10 +17,11 @@ package io.grpc.gcp.observability; import io.grpc.Internal; -import io.grpc.observabilitylog.v1.GrpcLogRecord.EventType; import io.opencensus.trace.Sampler; import java.util.List; import java.util.Map; +import java.util.Set; +import javax.annotation.concurrent.ThreadSafe; @Internal public interface ObservabilityConfig { @@ -33,14 +34,14 @@ public interface ObservabilityConfig { /** Is Cloud Tracing enabled. */ boolean isEnableCloudTracing(); - /** Get destination project ID - where logs will go. */ - String getDestinationProjectId(); + /** Get project ID - where logs will go. */ + String getProjectId(); - /** Get filters set for logging. */ - List getLogFilters(); + /** Get filters for client logging. */ + List getClientLogFilters(); - /** Get event types to log. */ - List getEventTypes(); + /** Get filters for server logging. */ + List getServerLogFilters(); /** Get sampler for TraceConfig - when Cloud Tracing is enabled. */ Sampler getSampler(); @@ -51,27 +52,49 @@ public interface ObservabilityConfig { /** * POJO for representing a filter used in configuration. */ + @ThreadSafe class LogFilter { - /** Pattern indicating which service/method to log. */ - public final String pattern; + /** Pattern for service/method filter. */ + public final List pattern; - /** Number of bytes of each header to log. */ + /** Boolean to indicate all services and methods. */ + public final Boolean matchAll; + + /** Set of services. */ + public final Set services; + + /* Set of fullMethodNames. */ + public final Set methods; + + /** Number of bytes of header to log. */ public final Integer headerBytes; - /** Number of bytes of each header to log. */ + /** Number of bytes of message to log. */ public final Integer messageBytes; + /** Boolean to indicate if services and methods matching pattern needs to be excluded. */ + public final Boolean excludePattern; + /** * Object used to represent filter used in configuration. - * - * @param pattern Pattern indicating which service/method to log - * @param headerBytes Number of bytes of each header to log - * @param messageBytes Number of bytes of each header to log + * @param pattern List of service/method filter + * @param matchAll If true, match all services and methods + * @param services Set of services derived from pattern + * @param serviceMethods Set of fullMethodNames derived from pattern + * @param headerBytes Total number of bytes of header to log + * @param messageBytes Total number of bytes of message to log + * @param excludePattern If true, services and methods matching pattern be excluded */ - public LogFilter(String pattern, Integer headerBytes, Integer messageBytes) { + public LogFilter(List pattern, Boolean matchAll, Set services, + Set serviceMethods, Integer headerBytes, Integer messageBytes, + Boolean excludePattern) { this.pattern = pattern; + this.matchAll = matchAll; + this.services = services; + this.methods = serviceMethods; this.headerBytes = headerBytes; this.messageBytes = messageBytes; + this.excludePattern = excludePattern; } } } diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java index 073916180a6..5225836a8fd 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java @@ -21,9 +21,10 @@ import com.google.common.base.Charsets; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import io.grpc.MethodDescriptor; import io.grpc.internal.JsonParser; import io.grpc.internal.JsonUtil; -import io.grpc.observabilitylog.v1.GrpcLogRecord.EventType; import io.opencensus.trace.Sampler; import io.opencensus.trace.samplers.Samplers; import java.io.IOException; @@ -31,22 +32,31 @@ import java.nio.file.Paths; import java.util.List; import java.util.Map; +import java.util.regex.Pattern; +import java.util.stream.Collectors; /** * gRPC GcpObservability configuration processor. */ final class ObservabilityConfigImpl implements ObservabilityConfig { - private static final String CONFIG_ENV_VAR_NAME = "GRPC_CONFIG_OBSERVABILITY"; - private static final String CONFIG_FILE_ENV_VAR_NAME = "GRPC_CONFIG_OBSERVABILITY_JSON"; + private static final String CONFIG_ENV_VAR_NAME = "GRPC_GCP_OBSERVABILITY_CONFIG"; + private static final String CONFIG_FILE_ENV_VAR_NAME = "GRPC_GCP_OBSERVABILITY_CONFIG_FILE"; + private static final String CONFIG_LOGGING_VAR_NAME = "cloud_logging"; + private static final String CONFIG_MONITORING_VAR_NAME = "cloud_monitoring"; + private static final String CONFIG_TRACING_VAR_NAME = "cloud_tracing"; + private static final String CONFIG_LABELS_VAR_NAME = "labels"; // Tolerance for floating-point comparisons. private static final double EPSILON = 1e-6; + private static final Pattern METHOD_NAME_REGEX = + Pattern.compile("^([\\w./]+)/((?:\\w+)|[*])$"); + private boolean enableCloudLogging = false; private boolean enableCloudMonitoring = false; private boolean enableCloudTracing = false; - private String destinationProjectId = null; - private List logFilters; - private List eventTypes; + private String projectId = null; + private List clientLogFilters; + private List serverLogFilters; private Sampler sampler; private Map customTags; @@ -62,7 +72,10 @@ static ObservabilityConfigImpl getInstance() throws IOException { } void parseFile(String configFile) throws IOException { - parse(new String(Files.readAllBytes(Paths.get(configFile)), Charsets.UTF_8)); + String configFileContent = + new String(Files.readAllBytes(Paths.get(configFile)), Charsets.UTF_8); + checkArgument(!configFileContent.isEmpty(), CONFIG_FILE_ENV_VAR_NAME + " is empty!"); + parse(configFileContent); } @SuppressWarnings("unchecked") @@ -73,72 +86,128 @@ void parse(String config) throws IOException { private void parseConfig(Map config) { if (config != null) { - Boolean value = JsonUtil.getBoolean(config, "enable_cloud_logging"); - if (value != null) { - enableCloudLogging = value; - } - value = JsonUtil.getBoolean(config, "enable_cloud_monitoring"); - if (value != null) { - enableCloudMonitoring = value; - } - value = JsonUtil.getBoolean(config, "enable_cloud_trace"); - if (value != null) { - enableCloudTracing = value; - } - destinationProjectId = JsonUtil.getString(config, "destination_project_id"); - List rawList = JsonUtil.getList(config, "log_filters"); - if (rawList != null) { - List> jsonLogFilters = JsonUtil.checkObjectList(rawList); - ImmutableList.Builder logFiltersBuilder = new ImmutableList.Builder<>(); - for (Map jsonLogFilter : jsonLogFilters) { - logFiltersBuilder.add(parseJsonLogFilter(jsonLogFilter)); - } - this.logFilters = logFiltersBuilder.build(); + projectId = JsonUtil.getString(config, "project_id"); + parseLoggingObject(config, CONFIG_LOGGING_VAR_NAME); + parseMonitoringObject(config, CONFIG_MONITORING_VAR_NAME); + parseTracingObject(config, CONFIG_TRACING_VAR_NAME); + parseCustomTags(config, CONFIG_LABELS_VAR_NAME); + } + } + + private void parseLoggingObject(Map config, String jsonObjectName) { + Map rawCloudLogging = JsonUtil.getObject(config, jsonObjectName); + if (rawCloudLogging != null) { + enableCloudLogging = true; + List rawClientRpcEvents = JsonUtil.getList(rawCloudLogging, "client_rpc_events"); + clientLogFilters = + rawClientRpcEvents != null ? parseRpcEvents(rawClientRpcEvents) : clientLogFilters; + List rawServerRpcEvents = JsonUtil.getList(rawCloudLogging, "server_rpc_events"); + serverLogFilters = + rawServerRpcEvents != null ? parseRpcEvents(rawServerRpcEvents) : serverLogFilters; + } + } + + private void parseMonitoringObject(Map config, String jsonObjectName) { + Map rawCloudMonitoring = JsonUtil.getObject(config, jsonObjectName); + if (rawCloudMonitoring != null) { + if (rawCloudMonitoring.isEmpty()) { + enableCloudMonitoring = true; } - rawList = JsonUtil.getList(config, "event_types"); - if (rawList != null) { - List jsonEventTypes = JsonUtil.checkStringList(rawList); - ImmutableList.Builder eventTypesBuilder = new ImmutableList.Builder<>(); - for (String jsonEventType : jsonEventTypes) { - eventTypesBuilder.add(EventType.valueOf(jsonEventType)); + } + } + + private void parseTracingObject(Map config, String jsonObjectName) { + Map rawCloudTracing = JsonUtil.getObject(config, jsonObjectName); + if (rawCloudTracing != null) { + enableCloudTracing = true; + this.sampler = Samplers.probabilitySampler(0.0); + if (!rawCloudTracing.isEmpty()) { + Double samplingRate = JsonUtil.getNumberAsDouble(rawCloudTracing, "sampling_rate"); + if (samplingRate != null) { + checkArgument( + samplingRate >= 0.0 && samplingRate <= 1.0, + "'sampling_rate' needs to be between [0.0, 1.0]"); + // Using alwaysSample() instead of probabilitySampler() because according to + // {@link io.opencensus.trace.samplers.ProbabilitySampler#shouldSample} + // there is a (very) small chance of *not* sampling if probability = 1.00. + this.sampler = + 1 - samplingRate < EPSILON + ? Samplers.alwaysSample() + : Samplers.probabilitySampler(samplingRate); } - this.eventTypes = eventTypesBuilder.build(); } - Double samplingRate = JsonUtil.getNumberAsDouble(config, "global_trace_sampling_rate"); - if (samplingRate == null) { - this.sampler = Samplers.probabilitySampler(0.0); - } else { + } + } + + private void parseCustomTags(Map config, String jsonObjectName) { + Map rawCustomTags = JsonUtil.getObject(config, jsonObjectName); + if (rawCustomTags != null) { + ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); + for (Map.Entry entry: rawCustomTags.entrySet()) { checkArgument( - samplingRate >= 0.0 && samplingRate <= 1.0, - "'global_trace_sampling_rate' needs to be between [0.0, 1.0]"); - // Using alwaysSample() instead of probabilitySampler() because according to - // {@link io.opencensus.trace.samplers.ProbabilitySampler#shouldSample} - // there is a (very) small chance of *not* sampling if probability = 1.00. - if (1 - samplingRate < EPSILON) { - this.sampler = Samplers.alwaysSample(); - } else { - this.sampler = Samplers.probabilitySampler(samplingRate); - } - } - Map rawCustomTags = JsonUtil.getObject(config, "custom_tags"); - if (rawCustomTags != null) { - ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); - for (Map.Entry entry: rawCustomTags.entrySet()) { - checkArgument( - entry.getValue() instanceof String, - "'custom_tags' needs to be a map of "); - builder.put(entry.getKey(), (String) entry.getValue()); - } - customTags = builder.build(); + entry.getValue() instanceof String, + "'labels' needs to be a map of "); + builder.put(entry.getKey(), (String) entry.getValue()); } + customTags = builder.build(); + } + } + + private List parseRpcEvents(List rpcEvents) { + List> jsonRpcEvents = JsonUtil.checkObjectList(rpcEvents); + ImmutableList.Builder rpcEventsListBuilder = + new ImmutableList.Builder<>(); + for (Map jsonClientRpcEvent : jsonRpcEvents) { + rpcEventsListBuilder.add(parseJsonLogFilter(jsonClientRpcEvent)); } + return rpcEventsListBuilder.build(); } private LogFilter parseJsonLogFilter(Map logFilterMap) { - return new LogFilter(JsonUtil.getString(logFilterMap, "pattern"), - JsonUtil.getNumberAsInteger(logFilterMap, "header_bytes"), - JsonUtil.getNumberAsInteger(logFilterMap, "message_bytes")); + Boolean exclude = JsonUtil.getBoolean(logFilterMap, "exclude"); + Boolean global = false; + ImmutableList.Builder patternListBuilder = + new ImmutableList.Builder<>(); + ImmutableSet.Builder servicesSetBuilder = + new ImmutableSet.Builder<>(); + ImmutableSet.Builder methodsSetBuilder = + new ImmutableSet.Builder<>(); + List methodsList = JsonUtil.getList(logFilterMap, "methods"); + if (methodsList != null) { + for (Object pattern : methodsList) { + String methodOrServicePattern = String.valueOf(pattern); + if (methodOrServicePattern != null) { + if (methodOrServicePattern.equals("*")) { + if (exclude != null) { + checkArgument( + !exclude, + "cannot have 'exclude' and '*' wildcard in the same filter"); + } + global = true; + } else { + checkArgument( + METHOD_NAME_REGEX.matcher(methodOrServicePattern).matches(), + "invalid service or method string : " + methodOrServicePattern); + if (methodOrServicePattern.endsWith("/*")) { + String service = MethodDescriptor.extractFullServiceName(methodOrServicePattern); + servicesSetBuilder.add(service); + } else { + methodsSetBuilder.add(methodOrServicePattern); + } + } + } + } + patternListBuilder.addAll(methodsList.stream() + .filter(element -> element instanceof String) + .map(element -> (String) element) + .collect(Collectors.toList())); + } + return new LogFilter(patternListBuilder.build(), + global, servicesSetBuilder.build(), methodsSetBuilder.build(), + JsonUtil.getNumberAsInteger(logFilterMap, "max_metadata_bytes"), + JsonUtil.getNumberAsInteger(logFilterMap, "max_message_bytes"), + exclude); } @Override @@ -157,18 +226,18 @@ public boolean isEnableCloudTracing() { } @Override - public String getDestinationProjectId() { - return destinationProjectId; + public String getProjectId() { + return projectId; } @Override - public List getLogFilters() { - return logFilters; + public List getClientLogFilters() { + return clientLogFilters; } @Override - public List getEventTypes() { - return eventTypes; + public List getServerLogFilters() { + return serverLogFilters; } @Override diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java index 38a3c80861a..3b88a0c81c1 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java @@ -16,51 +16,28 @@ package io.grpc.gcp.observability.interceptors; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import io.grpc.Internal; -import io.grpc.MethodDescriptor; import io.grpc.gcp.observability.ObservabilityConfig; import io.grpc.gcp.observability.ObservabilityConfig.LogFilter; -import io.grpc.observabilitylog.v1.GrpcLogRecord.EventType; -import java.util.HashMap; import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.logging.Level; -import java.util.logging.Logger; /** * Parses gRPC GcpObservability configuration filters for interceptors usage. */ @Internal public class ConfigFilterHelper { - - private static final Logger logger = Logger.getLogger(ConfigFilterHelper.class.getName()); - public static final FilterParams NO_FILTER_PARAMS = FilterParams.create(false, 0, 0); - public static final String globalPattern = "*"; private final ObservabilityConfig config; - @VisibleForTesting - boolean methodOrServiceFilterPresent; - // Flag to log every service and method - @VisibleForTesting - Map perServiceFilters; - @VisibleForTesting - Map perMethodFilters; - @VisibleForTesting - Set logEventTypeSet; @VisibleForTesting ConfigFilterHelper(ObservabilityConfig config) { this.config = config; - this.methodOrServiceFilterPresent = false; - this.perServiceFilters = new HashMap<>(); - this.perMethodFilters = new HashMap<>(); } /** @@ -69,82 +46,41 @@ public class ConfigFilterHelper { * @param config processed ObservabilityConfig object * @return helper instance for filtering */ - public static ConfigFilterHelper factory(ObservabilityConfig config) { - ConfigFilterHelper filterHelper = new ConfigFilterHelper(config); - if (config.isEnableCloudLogging()) { - filterHelper.setMethodOrServiceFilterMaps(); - filterHelper.setEventFilterSet(); - } - return filterHelper; + public static ConfigFilterHelper getInstance(ObservabilityConfig config) { + return new ConfigFilterHelper(config); } - @VisibleForTesting - void setMethodOrServiceFilterMaps() { - List logFilters = config.getLogFilters(); - if (logFilters == null) { - return; - } - Map perServiceFilters = new HashMap<>(); - Map perMethodFilters = new HashMap<>(); - - for (LogFilter currentFilter : logFilters) { - // '*' for global, 'service/*' for service glob, or 'service/method' for fully qualified - String methodOrServicePattern = currentFilter.pattern; - int currentHeaderBytes - = currentFilter.headerBytes != null ? currentFilter.headerBytes : 0; - int currentMessageBytes - = currentFilter.messageBytes != null ? currentFilter.messageBytes : 0; - if (methodOrServicePattern.equals("*")) { - // parse config for global, e.g. "*" - if (perServiceFilters.containsKey(globalPattern)) { - logger.log(Level.WARNING, "Duplicate entry : {0}", methodOrServicePattern); - continue; - } - FilterParams params = FilterParams.create(true, - currentHeaderBytes, currentMessageBytes); - perServiceFilters.put(globalPattern, params); - } else if (methodOrServicePattern.endsWith("/*")) { - // TODO(DNVindhya): check if service name is a valid string for a service name - // parse config for a service, e.g. "service/*" - String service = MethodDescriptor.extractFullServiceName(methodOrServicePattern); - if (perServiceFilters.containsKey(service)) { - logger.log(Level.WARNING, "Duplicate entry : {0)", methodOrServicePattern); - continue; - } - FilterParams params = FilterParams.create(true, - currentHeaderBytes, currentMessageBytes); - perServiceFilters.put(service, params); - } else { - // TODO(DNVVindhya): check if methodOrServicePattern is a valid full qualified method name - // parse pattern for a fully qualified method, e.g "service/method" - if (perMethodFilters.containsKey(methodOrServicePattern)) { - logger.log(Level.WARNING, "Duplicate entry : {0}", methodOrServicePattern); - continue; + /** + * Checks if the corresponding service/method passed needs to be logged according to user provided + * observability configuration. + * Filters are evaluated in text order, first match is used. + * + * @param fullMethodName the fully qualified name of the method + * @return FilterParams object 1. specifies if the corresponding method needs to be logged + * (log field will be set to true) 2. values of payload limits retrieved from configuration + */ + public FilterParams logRpcMethod(String fullMethodName, Boolean client) { + FilterParams params = NO_FILTER_PARAMS; + + int index = checkNotNull(fullMethodName, "fullMethodName").lastIndexOf('/'); + String serviceName = fullMethodName.substring(0, index); + List logFilters = + client ? config.getClientLogFilters() : config.getServerLogFilters(); + + for (LogFilter logFilter : logFilters) { + if (logFilter.matchAll + || logFilter.services.contains(serviceName) + || logFilter.methods.contains(fullMethodName)) { + if (logFilter.excludePattern) { + return params; } - FilterParams params = FilterParams.create(true, - currentHeaderBytes, currentMessageBytes); - perMethodFilters.put(methodOrServicePattern, params); + int currentHeaderBytes = logFilter.headerBytes != null ? logFilter.headerBytes : 0; + int currentMessageBytes = logFilter.messageBytes != null ? logFilter.messageBytes : 0; + return FilterParams.create(true, currentHeaderBytes, currentMessageBytes); } } - this.perServiceFilters = ImmutableMap.copyOf(perServiceFilters); - this.perMethodFilters = ImmutableMap.copyOf(perMethodFilters); - if (!perServiceFilters.isEmpty() || !perMethodFilters.isEmpty()) { - this.methodOrServiceFilterPresent = true; - } - } - - @VisibleForTesting - void setEventFilterSet() { - List eventFilters = config.getEventTypes(); - if (eventFilters == null) { - return; - } - if (eventFilters.isEmpty()) { - this.logEventTypeSet = ImmutableSet.of(); - return; - } - this.logEventTypeSet = ImmutableSet.copyOf(eventFilters); + return params; } /** @@ -166,50 +102,4 @@ public static FilterParams create(boolean log, int headerBytes, int messageBytes log, headerBytes, messageBytes); } } - - /** - * Checks if the corresponding service/method passed needs to be logged as per the user provided - * configuration. - * - * @param method the fully qualified name of the method - * @return MethodFilterParams object 1. specifies if the corresponding method needs to be logged - * (log field will be set to true) 2. values of payload limits retrieved from configuration - */ - public FilterParams isMethodToBeLogged(MethodDescriptor method) { - FilterParams params = NO_FILTER_PARAMS; - if (methodOrServiceFilterPresent) { - String fullMethodName = method.getFullMethodName(); - if (perMethodFilters.containsKey(fullMethodName)) { - params = perMethodFilters.get(fullMethodName); - } else { - String serviceName = method.getServiceName(); - if (perServiceFilters.containsKey(serviceName)) { - params = perServiceFilters.get(serviceName); - } else if (perServiceFilters.containsKey(globalPattern)) { - params = perServiceFilters.get(globalPattern); - } - } - } - return params; - } - - /** - * Checks if the corresponding event passed needs to be logged as per the user provided - * configuration. - * - *

All events are logged by default if event_types is not specified or {} in configuration. - * If event_types is specified as [], no events will be logged. - * If events types is specified as a non-empty list, only the events specified in the - * list will be logged. - *

- * - * @param event gRPC observability event - * @return true if event needs to be logged, false otherwise - */ - public boolean isEventToBeLogged(EventType event) { - if (logEventTypeSet == null) { - return true; - } - return logEventTypeSet.contains(event); - } } diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/InternalLoggingChannelInterceptor.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/InternalLoggingChannelInterceptor.java index e201827bce8..517745a5afc 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/InternalLoggingChannelInterceptor.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/InternalLoggingChannelInterceptor.java @@ -93,7 +93,7 @@ public ClientCall interceptCall(MethodDescriptor responseListener, Metadata headers) { final Duration timeout = deadline == null ? null : Durations.fromNanos(deadline.timeRemaining(TimeUnit.NANOSECONDS)); - if (filterHelper.isEventToBeLogged(EventType.CLIENT_HEADER)) { - try { - helper.logClientHeader( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - timeout, - headers, - maxHeaderBytes, - EventLogger.CLIENT, - callId, - null); - } catch (Exception e) { - // Catching generic exceptions instead of specific ones for all the events. - // This way we can catch both expected and unexpected exceptions instead of re-throwing - // exceptions to callers which will lead to RPC getting aborted. - // Expected exceptions to be caught: - // 1. IllegalArgumentException - // 2. NullPointerException - logger.log(Level.SEVERE, "Unable to log request header", e); - } + try { + helper.logClientHeader( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + timeout, + headers, + maxHeaderBytes, + EventLogger.CLIENT, + callId, + null); + } catch (Exception e) { + // Catching generic exceptions instead of specific ones for all the events. + // This way we can catch both expected and unexpected exceptions instead of re-throwing + // exceptions to callers which will lead to RPC getting aborted. + // Expected exceptions to be caught: + // 1. IllegalArgumentException + // 2. NullPointerException + logger.log(Level.SEVERE, "Unable to log request header", e); } Listener observabilityListener = @@ -140,22 +138,19 @@ public void start(Listener responseListener, Metadata headers) { @Override public void onMessage(RespT message) { // Event: EventType.SERVER_MESSAGE - EventType responseMessageType = EventType.SERVER_MESSAGE; - if (filterHelper.isEventToBeLogged(responseMessageType)) { - try { - helper.logRpcMessage( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - responseMessageType, - message, - maxMessageBytes, - EventLogger.CLIENT, - callId); - } catch (Exception e) { - logger.log(Level.SEVERE, "Unable to log response message", e); - } + try { + helper.logRpcMessage( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + EventType.SERVER_MESSAGE, + message, + maxMessageBytes, + EventLogger.CLIENT, + callId); + } catch (Exception e) { + logger.log(Level.SEVERE, "Unable to log response message", e); } super.onMessage(message); } @@ -163,21 +158,19 @@ public void onMessage(RespT message) { @Override public void onHeaders(Metadata headers) { // Event: EventType.SERVER_HEADER - if (filterHelper.isEventToBeLogged(EventType.SERVER_HEADER)) { - try { - helper.logServerHeader( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - headers, - maxHeaderBytes, - EventLogger.CLIENT, - callId, - LogHelper.getPeerAddress(getAttributes())); - } catch (Exception e) { - logger.log(Level.SEVERE, "Unable to log response header", e); - } + try { + helper.logServerHeader( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + headers, + maxHeaderBytes, + EventLogger.CLIENT, + callId, + LogHelper.getPeerAddress(getAttributes())); + } catch (Exception e) { + logger.log(Level.SEVERE, "Unable to log response header", e); } super.onHeaders(headers); } @@ -185,22 +178,20 @@ public void onHeaders(Metadata headers) { @Override public void onClose(Status status, Metadata trailers) { // Event: EventType.SERVER_TRAILER - if (filterHelper.isEventToBeLogged(EventType.SERVER_TRAILER)) { - try { - helper.logTrailer( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - status, - trailers, - maxHeaderBytes, - EventLogger.CLIENT, - callId, - LogHelper.getPeerAddress(getAttributes())); - } catch (Exception e) { - logger.log(Level.SEVERE, "Unable to log trailer", e); - } + try { + helper.logTrailer( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + status, + trailers, + maxHeaderBytes, + EventLogger.CLIENT, + callId, + LogHelper.getPeerAddress(getAttributes())); + } catch (Exception e) { + logger.log(Level.SEVERE, "Unable to log trailer", e); } super.onClose(status, trailers); } @@ -211,22 +202,19 @@ public void onClose(Status status, Metadata trailers) { @Override public void sendMessage(ReqT message) { // Event: EventType.CLIENT_MESSAGE - EventType requestMessageType = EventType.CLIENT_MESSAGE; - if (filterHelper.isEventToBeLogged(requestMessageType)) { - try { - helper.logRpcMessage( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - requestMessageType, - message, - maxMessageBytes, - EventLogger.CLIENT, - callId); - } catch (Exception e) { - logger.log(Level.SEVERE, "Unable to log request message", e); - } + try { + helper.logRpcMessage( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + EventType.CLIENT_MESSAGE, + message, + maxMessageBytes, + EventLogger.CLIENT, + callId); + } catch (Exception e) { + logger.log(Level.SEVERE, "Unable to log request message", e); } super.sendMessage(message); } @@ -234,18 +222,16 @@ public void sendMessage(ReqT message) { @Override public void halfClose() { // Event: EventType.CLIENT_HALF_CLOSE - if (filterHelper.isEventToBeLogged(EventType.CLIENT_HALF_CLOSE)) { - try { - helper.logHalfClose( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - EventLogger.CLIENT, - callId); - } catch (Exception e) { - logger.log(Level.SEVERE, "Unable to log half close", e); - } + try { + helper.logHalfClose( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + EventLogger.CLIENT, + callId); + } catch (Exception e) { + logger.log(Level.SEVERE, "Unable to log half close", e); } super.halfClose(); } @@ -253,18 +239,16 @@ public void halfClose() { @Override public void cancel(String message, Throwable cause) { // Event: EventType.CANCEL - if (filterHelper.isEventToBeLogged(EventType.CANCEL)) { - try { - helper.logCancel( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - EventLogger.CLIENT, - callId); - } catch (Exception e) { - logger.log(Level.SEVERE, "Unable to log cancel", e); - } + try { + helper.logCancel( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + EventLogger.CLIENT, + callId); + } catch (Exception e) { + logger.log(Level.SEVERE, "Unable to log cancel", e); } super.cancel(message, cause); } diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/InternalLoggingServerInterceptor.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/InternalLoggingServerInterceptor.java index 217674f13ca..fe98fbdc6d5 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/InternalLoggingServerInterceptor.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/InternalLoggingServerInterceptor.java @@ -93,7 +93,8 @@ public ServerCall.Listener interceptCall(ServerCall ServerCall.Listener interceptCall(ServerCall wrapperCall = @@ -131,21 +130,19 @@ public ServerCall.Listener interceptCall(ServerCall(listener) { @Override public void onMessage(ReqT message) { + // Event: EventType.CLIENT_MESSAGE EventType requestMessageType = EventType.CLIENT_MESSAGE; - if (filterHelper.isEventToBeLogged(requestMessageType)) { - try { - helper.logRpcMessage( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - requestMessageType, - message, - maxMessageBytes, - EventLogger.SERVER, - callId); - } catch (Exception e) { - logger.log(Level.SEVERE, "Unable to log request message", e); - } + try { + helper.logRpcMessage( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + requestMessageType, + message, + maxMessageBytes, + EventLogger.SERVER, + callId); + } catch (Exception e) { + logger.log(Level.SEVERE, "Unable to log request message", e); } super.onMessage(message); } @@ -225,18 +217,16 @@ public void onMessage(ReqT message) { @Override public void onHalfClose() { // Event: EventType.CLIENT_HALF_CLOSE - if (filterHelper.isEventToBeLogged(EventType.CLIENT_HALF_CLOSE)) { - try { - helper.logHalfClose( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - EventLogger.SERVER, - callId); - } catch (Exception e) { - logger.log(Level.SEVERE, "Unable to log half close", e); - } + try { + helper.logHalfClose( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + EventLogger.SERVER, + callId); + } catch (Exception e) { + logger.log(Level.SEVERE, "Unable to log half close", e); } super.onHalfClose(); } @@ -244,18 +234,16 @@ public void onHalfClose() { @Override public void onCancel() { // Event: EventType.CANCEL - if (filterHelper.isEventToBeLogged(EventType.CANCEL)) { - try { - helper.logCancel( - seq.getAndIncrement(), - serviceName, - methodName, - authority, - EventLogger.SERVER, - callId); - } catch (Exception e) { - logger.log(Level.SEVERE, "Unable to log cancel", e); - } + try { + helper.logCancel( + seq.getAndIncrement(), + serviceName, + methodName, + authority, + EventLogger.SERVER, + callId); + } catch (Exception e) { + logger.log(Level.SEVERE, "Unable to log cancel", e); } super.onCancel(); } diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java index afcaaea8ed4..e91f310e647 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java @@ -65,21 +65,22 @@ public class GcpLogSink implements Sink { private final Collection servicesToExclude; @VisibleForTesting - GcpLogSink(Logging loggingClient, String destinationProjectId, Map locationTags, + GcpLogSink(Logging loggingClient, String projectId, Map locationTags, Map customTags, Collection servicesToExclude) { - this(destinationProjectId, locationTags, customTags, servicesToExclude); + this(projectId, locationTags, customTags, servicesToExclude); this.gcpLoggingClient = loggingClient; } /** * Retrieves a single instance of GcpLogSink. - * @param destinationProjectId cloud project id to write logs + * + * @param projectId GCP project id to write logs * @param servicesToExclude service names for which log entries should not be generated */ - public GcpLogSink(String destinationProjectId, Map locationTags, + public GcpLogSink(String projectId, Map locationTags, Map customTags, Collection servicesToExclude) { - this.projectId = destinationProjectId; - this.customTags = getCustomTags(customTags, locationTags, destinationProjectId); + this.projectId = projectId; + this.customTags = getCustomTags(customTags, locationTags, projectId); this.kubernetesResource = getResource(locationTags); this.servicesToExclude = checkNotNull(servicesToExclude, "servicesToExclude"); } @@ -136,12 +137,12 @@ Logging createLoggingClient() { @VisibleForTesting static Map getCustomTags(Map customTags, - Map locationTags, String destinationProjectId) { + Map locationTags, String projectId) { ImmutableMap.Builder tagsBuilder = ImmutableMap.builder(); String sourceProjectId = locationTags.get("project_id"); - if (!Strings.isNullOrEmpty(destinationProjectId) + if (!Strings.isNullOrEmpty(projectId) && !Strings.isNullOrEmpty(sourceProjectId) - && !Objects.equals(sourceProjectId, destinationProjectId)) { + && !Objects.equals(sourceProjectId, projectId)) { tagsBuilder.put("source_project_id", sourceProjectId); } if (customTags != null) { diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java index 52267cf03df..913e2b75113 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java @@ -17,7 +17,8 @@ package io.grpc.gcp.observability; import static com.google.common.truth.Truth.assertThat; -import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verifyNoInteractions; @@ -25,7 +26,6 @@ import com.google.common.collect.ImmutableMap; import io.grpc.ManagedChannelBuilder; -import io.grpc.MethodDescriptor; import io.grpc.Server; import io.grpc.ServerBuilder; import io.grpc.StaticTestingClassLoader; @@ -36,8 +36,6 @@ import io.grpc.gcp.observability.interceptors.LogHelper; import io.grpc.gcp.observability.logging.GcpLogSink; import io.grpc.gcp.observability.logging.Sink; -import io.grpc.observabilitylog.v1.GrpcLogRecord; -import io.grpc.observabilitylog.v1.GrpcLogRecord.EventType; import io.grpc.testing.GrpcCleanupRule; import io.grpc.testing.protobuf.SimpleServiceGrpc; import java.io.IOException; @@ -97,13 +95,6 @@ public void clientServer_interceptorCalled_logNever() throws Exception { ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); } - @Test - public void clientServer_interceptorCalled_logFewEvents() throws Exception { - Class runnable = - classLoader.loadClass(LoggingTest.StaticTestingClassLogFewEvents.class.getName()); - ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); - } - // UsedReflectively public static final class StaticTestingClassEndtoEndLogging implements Runnable { @@ -122,9 +113,10 @@ public void run() { when(config.isEnableCloudLogging()).thenReturn(true); FilterParams logAlwaysFilterParams = FilterParams.create(true, 1024, 10); - when(mockFilterHelper.isMethodToBeLogged(any(MethodDescriptor.class))) + when(mockFilterHelper.logRpcMethod(anyString(), eq(true))) + .thenReturn(logAlwaysFilterParams); + when(mockFilterHelper.logRpcMethod(anyString(), eq(false))) .thenReturn(logAlwaysFilterParams); - when(mockFilterHelper.isEventToBeLogged(any(GrpcLogRecord.EventType.class))).thenReturn(true); try (GcpObservability unused = GcpObservability.grpcInit( @@ -163,9 +155,10 @@ public void run() { when(config.isEnableCloudLogging()).thenReturn(true); FilterParams logNeverFilterParams = FilterParams.create(false, 0, 0); - when(mockFilterHelper.isMethodToBeLogged(any(MethodDescriptor.class))) + when(mockFilterHelper.logRpcMethod(anyString(), eq(true))) + .thenReturn(logNeverFilterParams); + when(mockFilterHelper.logRpcMethod(anyString(), eq(false))) .thenReturn(logNeverFilterParams); - when(mockFilterHelper.isEventToBeLogged(any(GrpcLogRecord.EventType.class))).thenReturn(true); try (GcpObservability unused = GcpObservability.grpcInit( @@ -189,61 +182,4 @@ public void run() { } } } - - public static final class StaticTestingClassLogFewEvents implements Runnable { - - @Override - public void run() { - Sink mockSink = mock(GcpLogSink.class); - ObservabilityConfig config = mock(ObservabilityConfig.class); - LogHelper mockLogHelper = mock(LogHelper.class); - ConfigFilterHelper mockFilterHelper2 = mock(ConfigFilterHelper.class); - InternalLoggingChannelInterceptor.Factory channelInterceptorFactory = - new InternalLoggingChannelInterceptor.FactoryImpl(mockLogHelper, mockFilterHelper2); - InternalLoggingServerInterceptor.Factory serverInterceptorFactory = - new InternalLoggingServerInterceptor.FactoryImpl(mockLogHelper, mockFilterHelper2); - - when(config.isEnableCloudLogging()).thenReturn(true); - FilterParams logAlwaysFilterParams = FilterParams.create(true, 0, 0); - when(mockFilterHelper2.isMethodToBeLogged(any(MethodDescriptor.class))) - .thenReturn(logAlwaysFilterParams); - when(mockFilterHelper2.isEventToBeLogged(EventType.CLIENT_HEADER)) - .thenReturn(true); - when(mockFilterHelper2.isEventToBeLogged(EventType.SERVER_HEADER)) - .thenReturn(true); - when(mockFilterHelper2.isEventToBeLogged(EventType.CLIENT_HALF_CLOSE)).thenReturn(true); - when(mockFilterHelper2.isEventToBeLogged(EventType.SERVER_TRAILER)).thenReturn(true); - when(mockFilterHelper2.isEventToBeLogged(EventType.CANCEL)).thenReturn(true); - when(mockFilterHelper2.isEventToBeLogged(EventType.CLIENT_MESSAGE)) - .thenReturn(false); - when(mockFilterHelper2.isEventToBeLogged(EventType.SERVER_MESSAGE)) - .thenReturn(false); - - try (GcpObservability observability = - GcpObservability.grpcInit( - mockSink, config, channelInterceptorFactory, serverInterceptorFactory)) { - Server server = - ServerBuilder.forPort(0) - .addService(new LoggingTestHelper.SimpleServiceImpl()) - .build() - .start(); - int port = cleanupRule.register(server).getPort(); - SimpleServiceGrpc.SimpleServiceBlockingStub stub = - SimpleServiceGrpc.newBlockingStub( - cleanupRule.register( - ManagedChannelBuilder.forAddress("localhost", port).usePlaintext().build())); - assertThat(LoggingTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) - .isEqualTo("Hello buddy"); - // Total number of calls should have been 14 (6 from client and 6 from server) - // Since cancel is not invoked, it will be 12. - // Request message(Total count:2 (1 from client and 1 from server) and Response - // message(count:2) - // events are not in the event_types list, i.e 14 - 2(cancel) - 2(req_msg) - 2(resp_msg) - // = 8 - assertThat(Mockito.mockingDetails(mockLogHelper).getInvocations().size()).isEqualTo(8); - } catch (IOException e) { - throw new AssertionError("Exception while testing logging event filter", e); - } - } - } } diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/MetricsTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/MetricsTest.java index f967b99fbcb..645a4027bf7 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/MetricsTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/MetricsTest.java @@ -97,7 +97,7 @@ public void run() { mock(InternalLoggingServerInterceptor.Factory.class); when(mockConfig.isEnableCloudMonitoring()).thenReturn(true); - when(mockConfig.getDestinationProjectId()).thenReturn(PROJECT_ID); + when(mockConfig.getProjectId()).thenReturn(PROJECT_ID); try { GcpObservability observability = diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java index b2541d6a64a..3c624ab8cc8 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java @@ -23,18 +23,22 @@ import static org.junit.Assert.fail; import com.google.common.base.Charsets; -import com.google.common.collect.ImmutableList; import io.grpc.gcp.observability.ObservabilityConfig.LogFilter; -import io.grpc.observabilitylog.v1.GrpcLogRecord.EventType; import io.opencensus.trace.Sampler; import io.opencensus.trace.samplers.Samplers; import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Paths; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -43,82 +47,145 @@ @RunWith(JUnit4.class) public class ObservabilityConfigImplTest { - private static final String EVENT_TYPES = "{\n" - + " \"enable_cloud_logging\": false,\n" - + " \"event_types\": " - + "[\"CLIENT_HEADER\", \"CLIENT_HALF_CLOSE\", \"SERVER_TRAILER\"]\n" + private static final String LOG_FILTERS = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + + " \"cloud_logging\": {\n" + + " \"client_rpc_events\": [{\n" + + " \"methods\": [\"*\"],\n" + + " \"max_metadata_bytes\": 4096\n" + + " }" + + " ],\n" + + " \"server_rpc_events\": [{\n" + + " \"methods\": [\"*\"],\n" + + " \"max_metadata_bytes\": 32,\n" + + " \"max_message_bytes\": 64\n" + + " }" + + " ]\n" + + " }\n" + "}"; - private static final String LOG_FILTERS = "{\n" - + " \"enable_cloud_logging\": true,\n" - + " \"destination_project_id\": \"grpc-testing\",\n" - + " \"log_filters\": [{\n" - + " \"pattern\": \"*/*\",\n" - + " \"header_bytes\": 4096,\n" - + " \"message_bytes\": 2048\n" + private static final String CLIENT_LOG_FILTERS = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + + " \"cloud_logging\": {\n" + + " \"client_rpc_events\": [{\n" + + " \"methods\": [\"*\"],\n" + + " \"max_metadata_bytes\": 4096,\n" + + " \"max_message_bytes\": 2048\n" + " }," + " {\n" - + " \"pattern\": \"service1/Method2\"\n" + + " \"methods\": [\"service1/Method2\", \"Service2/*\"],\n" + + " \"exclude\": true\n" + " }" + " ]\n" + + " }\n" + "}"; - private static final String DEST_PROJECT_ID = "{\n" - + " \"enable_cloud_logging\": true,\n" - + " \"destination_project_id\": \"grpc-testing\"\n" - + "}"; + private static final String SERVER_LOG_FILTERS = "{\n" + + " \"cloud_logging\": {\n" + + " \"server_rpc_events\": [{\n" + + " \"methods\": [\"service1/method4\", \"service2/method234\"],\n" + + " \"max_metadata_bytes\": 32,\n" + + " \"max_message_bytes\": 64\n" + + " }," + + " {\n" + + " \"methods\": [\"service4/*\", \"Service2/*\"],\n" + + " \"exclude\": true\n" + + " }" + + " ]\n" + + " }\n" + + "}"; - private static final String DISABLE_CLOUD_LOGGING = "{\n" - + " \"enable_cloud_logging\": false\n" + private static final String PROJECT_ID = "{\n" + + " \"cloud_logging\": {},\n" + + " \"project_id\": \"grpc-testing\"\n" + "}"; + private static final String DISABLE_OBSERVABILITY = "{}"; + private static final String ENABLE_CLOUD_MONITORING_AND_TRACING = "{\n" - + " \"enable_cloud_monitoring\": true,\n" - + " \"enable_cloud_trace\": true\n" + + " \"cloud_monitoring\": {},\n" + + " \"cloud_tracing\": {}\n" + "}"; - private static final String GLOBAL_TRACING_ALWAYS_SAMPLER = "{\n" - + " \"enable_cloud_trace\": true,\n" - + " \"global_trace_sampling_rate\": 1.00\n" + private static final String ENABLE_CLOUD_MONITORING = "{\n" + + " \"cloud_monitoring\": {}\n" + "}"; - private static final String GLOBAL_TRACING_NEVER_SAMPLER = "{\n" - + " \"enable_cloud_trace\": true,\n" - + " \"global_trace_sampling_rate\": 0.00\n" + private static final String ENABLE_CLOUD_TRACING = "{\n" + + " \"cloud_tracing\": {}\n" + "}"; - private static final String GLOBAL_TRACING_PROBABILISTIC_SAMPLER = "{\n" - + " \"enable_cloud_trace\": true,\n" - + " \"global_trace_sampling_rate\": 0.75\n" + private static final String TRACING_ALWAYS_SAMPLER = "{\n" + + " \"cloud_tracing\": {\n" + + " \"sampling_rate\": 1.00\n" + + " }\n" + "}"; - private static final String GLOBAL_TRACING_DEFAULT_SAMPLER = "{\n" - + " \"enable_cloud_trace\": true\n" + private static final String TRACING_NEVER_SAMPLER = "{\n" + + " \"cloud_tracing\": {\n" + + " \"sampling_rate\": 0.00\n" + + " }\n" + "}"; - private static final String GLOBAL_TRACING_BADPROBABILISTIC_SAMPLER = "{\n" - + " \"enable_cloud_tracing\": true,\n" - + " \"global_trace_sampling_rate\": -0.75\n" + private static final String TRACING_PROBABILISTIC_SAMPLER = "{\n" + + " \"cloud_tracing\": {\n" + + " \"sampling_rate\": 0.75\n" + + " }\n" + + "}"; + + private static final String TRACING_DEFAULT_SAMPLER = "{\n" + + " \"cloud_tracing\": {}\n" + + "}"; + + private static final String GLOBAL_TRACING_BAD_PROBABILISTIC_SAMPLER = "{\n" + + " \"cloud_tracing\": {\n" + + " \"sampling_rate\": -0.75\n" + + " }\n" + "}"; private static final String CUSTOM_TAGS = "{\n" - + " \"enable_cloud_logging\": true,\n" - + " \"custom_tags\": {\n" + + " \"cloud_logging\": {},\n" + + " \"labels\": {\n" + " \"SOURCE_VERSION\" : \"J2e1Cf\",\n" + " \"SERVICE_NAME\" : \"payment-service\",\n" + " \"ENTRYPOINT_SCRIPT\" : \"entrypoint.sh\"\n" + " }\n" + "}"; - private static final String BAD_CUSTOM_TAGS = "{\n" - + " \"enable_cloud_monitoring\": true,\n" - + " \"custom_tags\": {\n" + private static final String BAD_CUSTOM_TAGS = + "{\n" + + " \"cloud_monitoring\": {},\n" + + " \"labels\": {\n" + " \"SOURCE_VERSION\" : \"J2e1Cf\",\n" + " \"SERVICE_NAME\" : { \"SUB_SERVICE_NAME\" : \"payment-service\"},\n" + " \"ENTRYPOINT_SCRIPT\" : \"entrypoint.sh\"\n" + " }\n" + "}"; + private static final String LOG_FILTER_GLOBAL_EXCLUDE = + "{\n" + + " \"cloud_logging\": {\n" + + " \"client_rpc_events\": [{\n" + + " \"methods\": [\"service1/Method2\", \"*\"],\n" + + " \"max_metadata_bytes\": 20,\n" + + " \"max_message_bytes\": 15,\n" + + " \"exclude\": true\n" + + " }" + + " ]\n" + + " }\n" + + "}"; + + private static final String LOG_FILTER_INVALID_METHOD = + "{\n" + + " \"cloud_logging\": {\n" + + " \"client_rpc_events\": [{\n" + + " \"methods\": [\"s*&%ervice1/Method2\", \"*\"],\n" + + " \"max_metadata_bytes\": 20\n" + + " }" + + " ]\n" + + " }\n" + + "}"; + ObservabilityConfigImpl observabilityConfig = new ObservabilityConfigImpl(); @Rule public TemporaryFolder tempFolder = new TemporaryFolder(); @@ -129,7 +196,7 @@ public void nullConfig() throws IOException { observabilityConfig.parse(null); fail("exception expected!"); } catch (IllegalArgumentException iae) { - assertThat(iae.getMessage()).isEqualTo("GRPC_CONFIG_OBSERVABILITY value is null!"); + assertThat(iae.getMessage()).isEqualTo("GRPC_GCP_OBSERVABILITY_CONFIG value is null!"); } } @@ -139,52 +206,138 @@ public void emptyConfig() throws IOException { assertFalse(observabilityConfig.isEnableCloudLogging()); assertFalse(observabilityConfig.isEnableCloudMonitoring()); assertFalse(observabilityConfig.isEnableCloudTracing()); - assertNull(observabilityConfig.getDestinationProjectId()); - assertNull(observabilityConfig.getLogFilters()); - assertNull(observabilityConfig.getEventTypes()); + assertNull(observabilityConfig.getProjectId()); + assertThat(observabilityConfig.getClientLogFilters()).isNull(); + assertThat(observabilityConfig.getServerLogFilters()).isNull(); + assertThat(observabilityConfig.getSampler()).isNull(); + assertThat(observabilityConfig.getCustomTags()).isNull(); } @Test - public void disableCloudLogging() throws IOException { - observabilityConfig.parse(DISABLE_CLOUD_LOGGING); + public void emptyConfigFile() throws IOException { + File configFile = tempFolder.newFile(); + try { + observabilityConfig.parseFile(configFile.getAbsolutePath()); + fail("exception expected!"); + } catch (IllegalArgumentException iae) { + assertThat(iae.getMessage()).isEqualTo( + "GRPC_GCP_OBSERVABILITY_CONFIG_FILE is empty!"); + } + } + + @Test + public void disableObservability() throws IOException { + observabilityConfig.parse(DISABLE_OBSERVABILITY); assertFalse(observabilityConfig.isEnableCloudLogging()); assertFalse(observabilityConfig.isEnableCloudMonitoring()); assertFalse(observabilityConfig.isEnableCloudTracing()); - assertNull(observabilityConfig.getDestinationProjectId()); - assertNull(observabilityConfig.getLogFilters()); - assertNull(observabilityConfig.getEventTypes()); + assertNull(observabilityConfig.getProjectId()); + assertThat(observabilityConfig.getClientLogFilters()).isNull(); + assertThat(observabilityConfig.getServerLogFilters()).isNull(); + assertThat(observabilityConfig.getSampler()).isNull(); + assertThat(observabilityConfig.getCustomTags()).isNull(); } @Test - public void destProjectId() throws IOException { - observabilityConfig.parse(DEST_PROJECT_ID); + public void setProjectId() throws IOException { + observabilityConfig.parse(PROJECT_ID); assertTrue(observabilityConfig.isEnableCloudLogging()); - assertThat(observabilityConfig.getDestinationProjectId()).isEqualTo("grpc-testing"); + assertThat(observabilityConfig.getProjectId()).isEqualTo("grpc-testing"); } @Test public void logFilters() throws IOException { observabilityConfig.parse(LOG_FILTERS); assertTrue(observabilityConfig.isEnableCloudLogging()); - assertThat(observabilityConfig.getDestinationProjectId()).isEqualTo("grpc-testing"); - List logFilters = observabilityConfig.getLogFilters(); - assertThat(logFilters).hasSize(2); - assertThat(logFilters.get(0).pattern).isEqualTo("*/*"); - assertThat(logFilters.get(0).headerBytes).isEqualTo(4096); - assertThat(logFilters.get(0).messageBytes).isEqualTo(2048); - assertThat(logFilters.get(1).pattern).isEqualTo("service1/Method2"); - assertThat(logFilters.get(1).headerBytes).isNull(); - assertThat(logFilters.get(1).messageBytes).isNull(); + assertThat(observabilityConfig.getProjectId()).isEqualTo("grpc-testing"); + + List clientLogFilters = observabilityConfig.getClientLogFilters(); + assertThat(clientLogFilters).hasSize(1); + assertThat(clientLogFilters.get(0).pattern).isEqualTo(Arrays.asList("*")); + assertThat(clientLogFilters.get(0).headerBytes).isEqualTo(4096); + assertThat(clientLogFilters.get(0).messageBytes).isNull(); + assertThat(clientLogFilters.get(0).excludePattern).isNull(); + assertThat(clientLogFilters.get(0).matchAll).isTrue(); + assertThat(clientLogFilters.get(0).services).isEmpty(); + assertThat(clientLogFilters.get(0).methods).isEmpty(); + + List serverLogFilters = observabilityConfig.getServerLogFilters(); + assertThat(serverLogFilters).hasSize(1); + assertThat(serverLogFilters.get(0).pattern).isEqualTo(Arrays.asList("*")); + assertThat(serverLogFilters.get(0).headerBytes).isEqualTo(32); + assertThat(serverLogFilters.get(0).messageBytes).isEqualTo(64); + assertThat(serverLogFilters.get(0).excludePattern).isNull(); + assertThat(serverLogFilters.get(0).matchAll).isTrue(); + assertThat(serverLogFilters.get(0).services).isEmpty(); + assertThat(serverLogFilters.get(0).methods).isEmpty(); } @Test - public void eventTypes() throws IOException { - observabilityConfig.parse(EVENT_TYPES); - assertFalse(observabilityConfig.isEnableCloudLogging()); - List eventTypes = observabilityConfig.getEventTypes(); - assertThat(eventTypes).isEqualTo( - ImmutableList.of(EventType.CLIENT_HEADER, EventType.CLIENT_HALF_CLOSE, - EventType.SERVER_TRAILER)); + public void setClientLogFilters() throws IOException { + observabilityConfig.parse(CLIENT_LOG_FILTERS); + assertTrue(observabilityConfig.isEnableCloudLogging()); + assertThat(observabilityConfig.getProjectId()).isEqualTo("grpc-testing"); + List logFilterList = observabilityConfig.getClientLogFilters(); + assertThat(logFilterList).hasSize(2); + assertThat(logFilterList.get(0).pattern).isEqualTo(Arrays.asList("*")); + assertThat(logFilterList.get(0).headerBytes).isEqualTo(4096); + assertThat(logFilterList.get(0).messageBytes).isEqualTo(2048); + assertThat(logFilterList.get(0).excludePattern).isNull(); + assertThat(logFilterList.get(0).matchAll).isTrue(); + assertThat(logFilterList.get(0).services).isEmpty(); + assertThat(logFilterList.get(0).methods).isEmpty(); + + assertThat(logFilterList.get(1).pattern) + .isEqualTo(Arrays.asList("service1/Method2", "Service2/*")); + assertThat(logFilterList.get(1).headerBytes).isNull(); + assertThat(logFilterList.get(1).messageBytes).isNull(); + assertThat(logFilterList.get(1).excludePattern).isTrue(); + assertThat(logFilterList.get(1).matchAll).isFalse(); + assertThat(logFilterList.get(1).services).isEqualTo(Collections.singleton("Service2")); + assertThat(logFilterList.get(1).methods) + .isEqualTo(Collections.singleton("service1/Method2")); + } + + @Test + public void setServerLogFilters() throws IOException { + Set expectedMethods = Stream.of("service1/method4", "service2/method234") + .collect(Collectors.toCollection(HashSet::new)); + observabilityConfig.parse(SERVER_LOG_FILTERS); + assertTrue(observabilityConfig.isEnableCloudLogging()); + List logFilterList = observabilityConfig.getServerLogFilters(); + assertThat(logFilterList).hasSize(2); + assertThat(logFilterList.get(0).pattern) + .isEqualTo(Arrays.asList("service1/method4", "service2/method234")); + assertThat(logFilterList.get(0).headerBytes).isEqualTo(32); + assertThat(logFilterList.get(0).messageBytes).isEqualTo(64); + assertThat(logFilterList.get(0).excludePattern).isNull(); + assertThat(logFilterList.get(0).matchAll).isFalse(); + assertThat(logFilterList.get(0).services).isEmpty(); + assertThat(logFilterList.get(0).methods) + .isEqualTo(expectedMethods); + + Set expectedServices = Stream.of("service4", "Service2") + .collect(Collectors.toCollection(HashSet::new)); + assertThat(logFilterList.get(1).pattern) + .isEqualTo(Arrays.asList("service4/*", "Service2/*")); + assertThat(logFilterList.get(1).headerBytes).isNull(); + assertThat(logFilterList.get(1).messageBytes).isNull(); + assertThat(logFilterList.get(1).excludePattern).isTrue(); + assertThat(logFilterList.get(1).matchAll).isFalse(); + assertThat(logFilterList.get(1).services).isEqualTo(expectedServices); + assertThat(logFilterList.get(1).methods).isEmpty(); + } + + @Test + public void enableCloudMonitoring() throws IOException { + observabilityConfig.parse(ENABLE_CLOUD_MONITORING); + assertTrue(observabilityConfig.isEnableCloudMonitoring()); + } + + @Test + public void enableCloudTracing() throws IOException { + observabilityConfig.parse(ENABLE_CLOUD_TRACING); + assertTrue(observabilityConfig.isEnableCloudTracing()); } @Test @@ -197,7 +350,7 @@ public void enableCloudMonitoringAndTracing() throws IOException { @Test public void alwaysSampler() throws IOException { - observabilityConfig.parse(GLOBAL_TRACING_ALWAYS_SAMPLER); + observabilityConfig.parse(TRACING_ALWAYS_SAMPLER); assertTrue(observabilityConfig.isEnableCloudTracing()); Sampler sampler = observabilityConfig.getSampler(); assertThat(sampler).isNotNull(); @@ -206,7 +359,7 @@ public void alwaysSampler() throws IOException { @Test public void neverSampler() throws IOException { - observabilityConfig.parse(GLOBAL_TRACING_NEVER_SAMPLER); + observabilityConfig.parse(TRACING_NEVER_SAMPLER); assertTrue(observabilityConfig.isEnableCloudTracing()); Sampler sampler = observabilityConfig.getSampler(); assertThat(sampler).isNotNull(); @@ -215,7 +368,7 @@ public void neverSampler() throws IOException { @Test public void probabilisticSampler() throws IOException { - observabilityConfig.parse(GLOBAL_TRACING_PROBABILISTIC_SAMPLER); + observabilityConfig.parse(TRACING_PROBABILISTIC_SAMPLER); assertTrue(observabilityConfig.isEnableCloudTracing()); Sampler sampler = observabilityConfig.getSampler(); assertThat(sampler).isNotNull(); @@ -224,7 +377,7 @@ public void probabilisticSampler() throws IOException { @Test public void defaultSampler() throws IOException { - observabilityConfig.parse(GLOBAL_TRACING_DEFAULT_SAMPLER); + observabilityConfig.parse(TRACING_DEFAULT_SAMPLER); assertTrue(observabilityConfig.isEnableCloudTracing()); Sampler sampler = observabilityConfig.getSampler(); assertThat(sampler).isNotNull(); @@ -234,29 +387,50 @@ public void defaultSampler() throws IOException { @Test public void badProbabilisticSampler_error() throws IOException { try { - observabilityConfig.parse(GLOBAL_TRACING_BADPROBABILISTIC_SAMPLER); + observabilityConfig.parse(GLOBAL_TRACING_BAD_PROBABILISTIC_SAMPLER); fail("exception expected!"); } catch (IllegalArgumentException iae) { assertThat(iae.getMessage()).isEqualTo( - "'global_trace_sampling_rate' needs to be between [0.0, 1.0]"); + "'sampling_rate' needs to be between [0.0, 1.0]"); } } @Test public void configFileLogFilters() throws Exception { File configFile = tempFolder.newFile(); - Files.write(Paths.get(configFile.getAbsolutePath()), LOG_FILTERS.getBytes(Charsets.US_ASCII)); + Files.write( + Paths.get(configFile.getAbsolutePath()), CLIENT_LOG_FILTERS.getBytes(Charsets.US_ASCII)); observabilityConfig.parseFile(configFile.getAbsolutePath()); assertTrue(observabilityConfig.isEnableCloudLogging()); - assertThat(observabilityConfig.getDestinationProjectId()).isEqualTo("grpc-testing"); - List logFilters = observabilityConfig.getLogFilters(); + assertThat(observabilityConfig.getProjectId()).isEqualTo("grpc-testing"); + List logFilters = observabilityConfig.getClientLogFilters(); + assertThat(logFilters).hasSize(2); + assertThat(logFilters.get(0).pattern).isEqualTo(Arrays.asList("*")); + assertThat(logFilters.get(0).headerBytes).isEqualTo(4096); + assertThat(logFilters.get(0).messageBytes).isEqualTo(2048); + assertThat(logFilters.get(1).pattern) + .isEqualTo(Arrays.asList("service1/Method2", "Service2/*")); + assertThat(logFilters.get(1).headerBytes).isNull(); + assertThat(logFilters.get(1).messageBytes).isNull(); + assertThat(logFilters).hasSize(2); - assertThat(logFilters.get(0).pattern).isEqualTo("*/*"); + assertThat(logFilters.get(0).pattern).isEqualTo(Arrays.asList("*")); assertThat(logFilters.get(0).headerBytes).isEqualTo(4096); assertThat(logFilters.get(0).messageBytes).isEqualTo(2048); - assertThat(logFilters.get(1).pattern).isEqualTo("service1/Method2"); + assertThat(logFilters.get(0).excludePattern).isNull(); + assertThat(logFilters.get(0).matchAll).isTrue(); + assertThat(logFilters.get(0).services).isEmpty(); + assertThat(logFilters.get(0).methods).isEmpty(); + + assertThat(logFilters.get(1).pattern) + .isEqualTo(Arrays.asList("service1/Method2", "Service2/*")); assertThat(logFilters.get(1).headerBytes).isNull(); assertThat(logFilters.get(1).messageBytes).isNull(); + assertThat(logFilters.get(1).excludePattern).isTrue(); + assertThat(logFilters.get(1).matchAll).isFalse(); + assertThat(logFilters.get(1).services).isEqualTo(Collections.singleton("Service2")); + assertThat(logFilters.get(1).methods) + .isEqualTo(Collections.singleton("service1/Method2")); } @Test @@ -277,7 +451,29 @@ public void badCustomTags() throws IOException { fail("exception expected!"); } catch (IllegalArgumentException iae) { assertThat(iae.getMessage()).isEqualTo( - "'custom_tags' needs to be a map of "); + "'labels' needs to be a map of "); + } + } + + @Test + public void globalLogFilterExclude() throws IOException { + try { + observabilityConfig.parse(LOG_FILTER_GLOBAL_EXCLUDE); + fail("exception expected!"); + } catch (IllegalArgumentException iae) { + assertThat(iae.getMessage()).isEqualTo( + "cannot have 'exclude' and '*' wildcard in the same filter"); + } + } + + @Test + public void logFilterInvalidMethod() throws IOException { + try { + observabilityConfig.parse(LOG_FILTER_INVALID_METHOD); + fail("exception expected!"); + } catch (IllegalArgumentException iae) { + assertThat(iae.getMessage()).contains( + "invalid service or method string"); } } -} \ No newline at end of file +} diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java index ec759827737..534f1236850 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java @@ -100,7 +100,7 @@ public void run() { when(mockConfig.isEnableCloudTracing()).thenReturn(true); when(mockConfig.getSampler()).thenReturn(Samplers.alwaysSample()); - when(mockConfig.getDestinationProjectId()).thenReturn(PROJECT_ID); + when(mockConfig.getProjectId()).thenReturn(PROJECT_ID); try { GcpObservability observability = diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelperTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelperTest.java index 2da869472ef..5076a6b1646 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelperTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelperTest.java @@ -17,44 +17,36 @@ package io.grpc.gcp.observability.interceptors; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import io.grpc.MethodDescriptor; import io.grpc.gcp.observability.ObservabilityConfig; import io.grpc.gcp.observability.ObservabilityConfig.LogFilter; import io.grpc.gcp.observability.interceptors.ConfigFilterHelper.FilterParams; -import io.grpc.observabilitylog.v1.GrpcLogRecord.EventType; -import io.grpc.testing.TestMethodDescriptors; -import java.util.ArrayList; -import java.util.HashMap; +import java.util.Arrays; +import java.util.Collections; import java.util.List; -import java.util.Map; -import java.util.Set; import org.junit.Before; import org.junit.Test; public class ConfigFilterHelperTest { private static final ImmutableList configLogFilters = ImmutableList.of( - new LogFilter("service1/Method2",1024,1024), - new LogFilter("service2/*",2048,1024), - new LogFilter("*",128,128), - new LogFilter("service2/*",2048,1024)); - - private static final ImmutableList configEventTypes = - ImmutableList.of( - EventType.CLIENT_HEADER, - EventType.CLIENT_HALF_CLOSE, - EventType.SERVER_TRAILER); - - private final MethodDescriptor.Builder builder = TestMethodDescriptors.voidMethod() - .toBuilder(); - private MethodDescriptor method; + new LogFilter(Collections.singletonList("service1/Method2"), false, + Collections.emptySet(), Collections.singleton("service1/Method2"), + 1024, 1024, false), + new LogFilter( + Arrays.asList("service2/*, service4/method2"), false, + Collections.singleton("service2"), Collections.singleton("service4/method2"), + 2048, 1024, false), + new LogFilter( + Arrays.asList("service2/*, service4/method3"), false, + Collections.singleton("service2"), Collections.singleton("service4/method3"), + 2048, 1024, true), + new LogFilter( + Collections.singletonList("*"), true, Collections.emptySet(), Collections.emptySet(), + 128, 128, false)); private ObservabilityConfig mockConfig; private ConfigFilterHelper configFilterHelper; @@ -66,153 +58,98 @@ public void setup() { } @Test - public void disableCloudLogging_emptyLogFilters() { - when(mockConfig.isEnableCloudLogging()).thenReturn(false); - assertFalse(configFilterHelper.methodOrServiceFilterPresent); - assertThat(configFilterHelper.perServiceFilters).isEmpty(); - assertThat(configFilterHelper.perServiceFilters).isEmpty(); - assertThat(configFilterHelper.perMethodFilters).isEmpty(); - assertThat(configFilterHelper.logEventTypeSet).isNull(); - } - - @Test - public void enableCloudLogging_emptyLogFilters() { + public void enableCloudLogging_withoutLogFilters() { when(mockConfig.isEnableCloudLogging()).thenReturn(true); - when(mockConfig.getLogFilters()).thenReturn(null); - when(mockConfig.getEventTypes()).thenReturn(null); - configFilterHelper.setMethodOrServiceFilterMaps(); - configFilterHelper.setEventFilterSet(); - - assertFalse(configFilterHelper.methodOrServiceFilterPresent); - assertThat(configFilterHelper.perServiceFilters).isEmpty(); - assertThat(configFilterHelper.perServiceFilters).isEmpty(); - assertThat(configFilterHelper.perMethodFilters).isEmpty(); - assertThat(configFilterHelper.logEventTypeSet).isNull(); + assertThat(mockConfig.getClientLogFilters()).isEmpty(); + assertThat(mockConfig.getServerLogFilters()).isEmpty(); } @Test - public void enableCloudLogging_withLogFilters() { + public void checkMethodLog_withoutLogFilters() { when(mockConfig.isEnableCloudLogging()).thenReturn(true); - when(mockConfig.getLogFilters()).thenReturn(configLogFilters); - when(mockConfig.getEventTypes()).thenReturn(configEventTypes); - - configFilterHelper.setMethodOrServiceFilterMaps(); - configFilterHelper.setEventFilterSet(); - - assertTrue(configFilterHelper.methodOrServiceFilterPresent); + assertThat(mockConfig.getClientLogFilters()).isEmpty(); + assertThat(mockConfig.getServerLogFilters()).isEmpty(); - Map expectedServiceFilters = new HashMap<>(); - expectedServiceFilters.put("*", - FilterParams.create(true, 128, 128)); - expectedServiceFilters.put("service2", - FilterParams.create(true, 2048, 1024)); - assertThat(configFilterHelper.perServiceFilters).isEqualTo(expectedServiceFilters); - - Map expectedMethodFilters = new HashMap<>(); - expectedMethodFilters.put("service1/Method2", - FilterParams.create(true, 1024, 1024)); - assertThat(configFilterHelper.perMethodFilters).isEqualTo(expectedMethodFilters); - - Set expectedLogEventTypeSet = ImmutableSet.copyOf(configEventTypes); - assertThat(configFilterHelper.logEventTypeSet).isEqualTo(expectedLogEventTypeSet); + FilterParams expectedParams = + FilterParams.create(false, 0, 0); + FilterParams clientResultParams + = configFilterHelper.logRpcMethod("service3/Method3", true); + assertThat(clientResultParams).isEqualTo(expectedParams); + FilterParams serverResultParams + = configFilterHelper.logRpcMethod("service3/Method3", false); + assertThat(serverResultParams).isEqualTo(expectedParams); } @Test public void checkMethodAlwaysLogged() { - List sampleLogFilters = ImmutableList.of( - new LogFilter("*", 4096, 4096)); - when(mockConfig.getLogFilters()).thenReturn(sampleLogFilters); - configFilterHelper.setMethodOrServiceFilterMaps(); + List sampleLogFilters = + ImmutableList.of( + new LogFilter( + Collections.singletonList("*"), true, Collections.emptySet(), + Collections.emptySet(), 4096, 4096, false)); + when(mockConfig.getClientLogFilters()).thenReturn(sampleLogFilters); + when(mockConfig.getServerLogFilters()).thenReturn(sampleLogFilters); FilterParams expectedParams = FilterParams.create(true, 4096, 4096); - method = builder.setFullMethodName("service1/Method6").build(); - FilterParams resultParams - = configFilterHelper.isMethodToBeLogged(method); - assertThat(resultParams).isEqualTo(expectedParams); + FilterParams clientResultParams + = configFilterHelper.logRpcMethod("service1/Method6", true); + assertThat(clientResultParams).isEqualTo(expectedParams); + FilterParams serverResultParams + = configFilterHelper.logRpcMethod("service1/Method6", false); + assertThat(serverResultParams).isEqualTo(expectedParams); } @Test public void checkMethodNotToBeLogged() { - List sampleLogFilters = ImmutableList.of( - new LogFilter("service1/Method2", 1024, 1024), - new LogFilter("service2/*", 2048, 1024)); - when(mockConfig.getLogFilters()).thenReturn(sampleLogFilters); - configFilterHelper.setMethodOrServiceFilterMaps(); + List sampleLogFilters = + ImmutableList.of( + new LogFilter(Collections.singletonList("service2/*"), false, + Collections.emptySet(), Collections.singleton("service2/*"), + 1024, 1024, true), + new LogFilter( + Collections.singletonList("service2/Method1"), false, + Collections.singleton("service2/Method1"), Collections.emptySet(), + 2048, 1024, false)); + when(mockConfig.getClientLogFilters()).thenReturn(sampleLogFilters); + when(mockConfig.getServerLogFilters()).thenReturn(sampleLogFilters); FilterParams expectedParams = FilterParams.create(false, 0, 0); - method = builder.setFullMethodName("service3/Method3").build(); - FilterParams resultParams - = configFilterHelper.isMethodToBeLogged(method); - assertThat(resultParams).isEqualTo(expectedParams); + FilterParams clientResultParams1 + = configFilterHelper.logRpcMethod("service3/Method3", true); + assertThat(clientResultParams1).isEqualTo(expectedParams); + + FilterParams clientResultParams2 + = configFilterHelper.logRpcMethod("service2/Method1", true); + assertThat(clientResultParams2).isEqualTo(expectedParams); + + FilterParams serverResultParams + = configFilterHelper.logRpcMethod("service2/Method1", false); + assertThat(serverResultParams).isEqualTo(expectedParams); } @Test public void checkMethodToBeLoggedConditional() { - when(mockConfig.getLogFilters()).thenReturn(configLogFilters); - configFilterHelper.setMethodOrServiceFilterMaps(); + when(mockConfig.getClientLogFilters()).thenReturn(configLogFilters); + when(mockConfig.getServerLogFilters()).thenReturn(configLogFilters); FilterParams expectedParams = FilterParams.create(true, 1024, 1024); - method = builder.setFullMethodName("service1/Method2").build(); FilterParams resultParams - = configFilterHelper.isMethodToBeLogged(method); + = configFilterHelper.logRpcMethod("service1/Method2", true); assertThat(resultParams).isEqualTo(expectedParams); FilterParams expectedParamsWildCard = FilterParams.create(true, 2048, 1024); - method = builder.setFullMethodName("service2/Method1").build(); FilterParams resultParamsWildCard - = configFilterHelper.isMethodToBeLogged(method); + = configFilterHelper.logRpcMethod("service2/Method1", true); assertThat(resultParamsWildCard).isEqualTo(expectedParamsWildCard); - } - - @Test - public void checkEventToBeLogged_noFilter_defaultLogAllEventTypes() { - List eventList = new ArrayList<>(); - eventList.add(EventType.CLIENT_HEADER); - eventList.add(EventType.SERVER_HEADER); - eventList.add(EventType.CLIENT_MESSAGE); - eventList.add(EventType.SERVER_MESSAGE); - eventList.add(EventType.CLIENT_HALF_CLOSE); - eventList.add(EventType.SERVER_TRAILER); - eventList.add(EventType.CANCEL); - - for (EventType event : eventList) { - assertTrue(configFilterHelper.isEventToBeLogged(event)); - } - } - - @Test - public void checkEventToBeLogged_emptyFilter_doNotLogEventTypes() { - when(mockConfig.getEventTypes()).thenReturn(new ArrayList<>()); - configFilterHelper.setEventFilterSet(); - - List eventList = new ArrayList<>(); - eventList.add(EventType.CLIENT_HEADER); - eventList.add(EventType.SERVER_HEADER); - eventList.add(EventType.CLIENT_MESSAGE); - eventList.add(EventType.SERVER_MESSAGE); - eventList.add(EventType.CLIENT_HALF_CLOSE); - eventList.add(EventType.SERVER_TRAILER); - eventList.add(EventType.CANCEL); - - for (EventType event : eventList) { - assertFalse(configFilterHelper.isEventToBeLogged(event)); - } - } - - @Test - public void checkEventToBeLogged_withEventTypesFromConfig() { - when(mockConfig.getEventTypes()).thenReturn(configEventTypes); - configFilterHelper.setEventFilterSet(); - - EventType logEventType = EventType.CLIENT_HEADER; - assertTrue(configFilterHelper.isEventToBeLogged(logEventType)); - - EventType doNotLogEventType = EventType.SERVER_MESSAGE; - assertFalse(configFilterHelper.isEventToBeLogged(doNotLogEventType)); + FilterParams excludeParams = + FilterParams.create(false, 0, 0); + FilterParams serverResultParams + = configFilterHelper.logRpcMethod("service4/method3", false); + assertThat(serverResultParams).isEqualTo(excludeParams); } } diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/InternalLoggingChannelInterceptorTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/InternalLoggingChannelInterceptorTest.java index 082bca826c5..2a2e1d4c229 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/InternalLoggingChannelInterceptorTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/InternalLoggingChannelInterceptorTest.java @@ -26,7 +26,6 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -108,8 +107,6 @@ public void setup() throws Exception { cancelCalled = SettableFuture.create(); peer = new InetSocketAddress(InetAddress.getByName("127.0.0.1"), 1234); filterParams = FilterParams.create(true, 0, 0); - when(mockFilterHelper.isEventToBeLogged(any(GrpcLogRecord.EventType.class))) - .thenReturn(true); } @Test @@ -164,7 +161,7 @@ public String authority() { .setRequestMarshaller(BYTEARRAY_MARSHALLER) .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); - when(mockFilterHelper.isMethodToBeLogged(method)) + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), true)) .thenReturn(filterParams); ClientCall interceptedLoggingCall = @@ -329,7 +326,7 @@ public void clientDeadLineLogged_deadlineSetViaCallOption() { .setRequestMarshaller(BYTEARRAY_MARSHALLER) .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); - when(mockFilterHelper.isMethodToBeLogged(method)) + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), true)) .thenReturn(filterParams); @SuppressWarnings("unchecked") ClientCall.Listener mockListener = mock(ClientCall.Listener.class); @@ -387,7 +384,7 @@ public void clientDeadlineLogged_deadlineSetViaContext() throws Exception { .setRequestMarshaller(BYTEARRAY_MARSHALLER) .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); - when(mockFilterHelper.isMethodToBeLogged(method)) + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), true)) .thenReturn(filterParams); callFuture.set( @@ -449,7 +446,7 @@ public void clientDeadlineLogged_deadlineSetViaContextAndCallOptions() throws Ex .setRequestMarshaller(BYTEARRAY_MARSHALLER) .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); - when(mockFilterHelper.isMethodToBeLogged(method)) + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), true)) .thenReturn(filterParams); callFuture.set( @@ -547,7 +544,7 @@ public String authority() { .setRequestMarshaller(BYTEARRAY_MARSHALLER) .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); - when(mockFilterHelper.isMethodToBeLogged(method)) + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), true)) .thenReturn(FilterParams.create(false, 0, 0)); ClientCall interceptedLoggingCall = @@ -612,7 +609,7 @@ public String authority() { .setRequestMarshaller(BYTEARRAY_MARSHALLER) .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); - when(mockFilterHelper.isMethodToBeLogged(method)) + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), true)) .thenReturn(FilterParams.create(true, 10, 10)); ClientCall interceptedLoggingCall = @@ -636,106 +633,4 @@ public String authority() { assertThat(Mockito.mockingDetails(mockLogHelper).getInvocations().size()).isEqualTo(7); } } - - @Test - public void eventFilter_enabled() { - when(mockFilterHelper.isEventToBeLogged(EventType.CLIENT_HEADER)).thenReturn(false); - when(mockFilterHelper.isEventToBeLogged(EventType.SERVER_HEADER)).thenReturn(false); - - Channel channel = new Channel() { - @Override - public ClientCall newCall( - MethodDescriptor methodDescriptor, CallOptions callOptions) { - return new NoopClientCall() { - @Override - @SuppressWarnings("unchecked") - public void start(Listener responseListener, Metadata headers) { - interceptedListener.set((Listener) responseListener); - actualClientInitial.set(headers); - } - - @Override - public void sendMessage(RequestT message) { - actualRequest.set(message); - } - - @Override - public void cancel(String message, Throwable cause) { - cancelCalled.set(null); - } - - @Override - public void halfClose() { - halfCloseCalled.set(null); - } - - @Override - public Attributes getAttributes() { - return Attributes.newBuilder().set(Grpc.TRANSPORT_ATTR_REMOTE_ADDR, peer).build(); - } - }; - } - - @Override - public String authority() { - return "the-authority"; - } - }; - - @SuppressWarnings("unchecked") - ClientCall.Listener mockListener = mock(ClientCall.Listener.class); - - MethodDescriptor method = - MethodDescriptor.newBuilder() - .setType(MethodType.UNKNOWN) - .setFullMethodName("service/method") - .setRequestMarshaller(BYTEARRAY_MARSHALLER) - .setResponseMarshaller(BYTEARRAY_MARSHALLER) - .build(); - when(mockFilterHelper.isMethodToBeLogged(method)) - .thenReturn(FilterParams.create(true, 10, 10)); - - ClientCall interceptedLoggingCall = - factory.create() - .interceptCall(method, - CallOptions.DEFAULT, - channel); - - { - interceptedLoggingCall.start(mockListener, new Metadata()); - verify(mockLogHelper, never()).logClientHeader( - anyLong(), - AdditionalMatchers.or(ArgumentMatchers.isNull(), anyString()), - AdditionalMatchers.or(ArgumentMatchers.isNull(), anyString()), - AdditionalMatchers.or(ArgumentMatchers.isNull(), anyString()), - any(Duration.class), - any(Metadata.class), - anyInt(), - any(GrpcLogRecord.EventLogger.class), - anyString(), - AdditionalMatchers.or(ArgumentMatchers.isNull(), - ArgumentMatchers.any())); - interceptedListener.get().onHeaders(new Metadata()); - verify(mockLogHelper, never()).logServerHeader( - anyLong(), - AdditionalMatchers.or(ArgumentMatchers.isNull(), anyString()), - AdditionalMatchers.or(ArgumentMatchers.isNull(), anyString()), - AdditionalMatchers.or(ArgumentMatchers.isNull(), anyString()), - any(Metadata.class), - anyInt(), - any(GrpcLogRecord.EventLogger.class), - anyString(), - ArgumentMatchers.any()); - byte[] request = "this is a request".getBytes(US_ASCII); - interceptedLoggingCall.sendMessage(request); - interceptedLoggingCall.halfClose(); - byte[] response = "this is a response".getBytes(US_ASCII); - interceptedListener.get().onMessage(response); - Status status = Status.INTERNAL.withDescription("trailer description"); - Metadata trailers = new Metadata(); - interceptedListener.get().onClose(status, trailers); - interceptedLoggingCall.cancel(null, null); - assertThat(Mockito.mockingDetails(mockLogHelper).getInvocations().size()).isEqualTo(5); - } - } } diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/InternalLoggingServerInterceptorTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/InternalLoggingServerInterceptorTest.java index d8a3bafbdb5..fee936dfbca 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/InternalLoggingServerInterceptorTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/InternalLoggingServerInterceptorTest.java @@ -20,12 +20,10 @@ import static io.grpc.gcp.observability.interceptors.LogHelperTest.BYTEARRAY_MARSHALLER; import static org.junit.Assert.assertSame; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -45,7 +43,6 @@ import io.grpc.Status; import io.grpc.gcp.observability.interceptors.ConfigFilterHelper.FilterParams; import io.grpc.internal.NoopServerCall; -import io.grpc.observabilitylog.v1.GrpcLogRecord; import io.grpc.observabilitylog.v1.GrpcLogRecord.EventLogger; import io.grpc.observabilitylog.v1.GrpcLogRecord.EventType; import java.net.InetAddress; @@ -61,7 +58,6 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -import org.mockito.AdditionalMatchers; import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatchers; import org.mockito.Mockito; @@ -105,8 +101,6 @@ public void setup() throws Exception { actualStatus = new AtomicReference<>(); actualTrailers = new AtomicReference<>(); peer = new InetSocketAddress(InetAddress.getByName("127.0.0.1"), 1234); - when(mockFilterHelper.isEventToBeLogged(any(GrpcLogRecord.EventType.class))) - .thenReturn(true); } @Test @@ -121,7 +115,7 @@ public void internalLoggingServerInterceptor() { .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); FilterParams filterParams = FilterParams.create(true, 0, 0); - when(mockFilterHelper.isMethodToBeLogged(method)).thenReturn(filterParams); + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), false)).thenReturn(filterParams); capturedListener = factory.create() .interceptCall( @@ -312,7 +306,7 @@ public void serverDeadlineLogged() { .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); FilterParams filterParams = FilterParams.create(true, 0, 0); - when(mockFilterHelper.isMethodToBeLogged(method)).thenReturn(filterParams); + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), false)).thenReturn(filterParams); final ServerCall noopServerCall = new NoopServerCall() { @Override public MethodDescriptor getMethodDescriptor() { @@ -365,7 +359,8 @@ public void serverMethodOrServiceFilter_disabled() { .setRequestMarshaller(BYTEARRAY_MARSHALLER) .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); - when(mockFilterHelper.isMethodToBeLogged(method)).thenReturn(FilterParams.create(false, 0, 0)); + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), false)) + .thenReturn(FilterParams.create(false, 0, 0)); capturedListener = factory.create() .interceptCall( @@ -422,7 +417,7 @@ public void serverMethodOrServiceFilter_enabled() { .setRequestMarshaller(BYTEARRAY_MARSHALLER) .setResponseMarshaller(BYTEARRAY_MARSHALLER) .build(); - when(mockFilterHelper.isMethodToBeLogged(method)) + when(mockFilterHelper.logRpcMethod(method.getFullMethodName(), false)) .thenReturn(FilterParams.create(true, 10, 10)); capturedListener = @@ -483,85 +478,4 @@ public String getAuthority() { assertThat(Mockito.mockingDetails(mockLogHelper).getInvocations().size()).isEqualTo(7); } } - - @Test - public void eventFilter_enabled() { - when(mockFilterHelper.isEventToBeLogged(EventType.CLIENT_HALF_CLOSE)).thenReturn(false); - - Metadata clientInitial = new Metadata(); - final MethodDescriptor method = - MethodDescriptor.newBuilder() - .setType(MethodType.UNKNOWN) - .setFullMethodName("service/method") - .setRequestMarshaller(BYTEARRAY_MARSHALLER) - .setResponseMarshaller(BYTEARRAY_MARSHALLER) - .build(); - when(mockFilterHelper.isMethodToBeLogged(method)) - .thenReturn(FilterParams.create(true, 10, 10)); - - capturedListener = - factory.create() - .interceptCall( - new NoopServerCall() { - @Override - public void sendHeaders(Metadata headers) { - actualServerInitial.set(headers); - } - - @Override - public void sendMessage(byte[] message) { - actualResponse.set(message); - } - - @Override - public void close(Status status, Metadata trailers) { - actualStatus.set(status); - actualTrailers.set(trailers); - } - - @Override - public MethodDescriptor getMethodDescriptor() { - return method; - } - - @Override - public Attributes getAttributes() { - return Attributes - .newBuilder() - .set(Grpc.TRANSPORT_ATTR_REMOTE_ADDR, peer) - .build(); - } - - @Override - public String getAuthority() { - return "the-authority"; - } - }, - clientInitial, - (call, headers) -> { - interceptedLoggingCall.set(call); - return mockListener; - }); - - { - interceptedLoggingCall.get().sendHeaders(new Metadata()); - byte[] request = "this is a request".getBytes(US_ASCII); - capturedListener.onMessage(request); - capturedListener.onHalfClose(); - byte[] response = "this is a response".getBytes(US_ASCII); - interceptedLoggingCall.get().sendMessage(response); - Status status = Status.INTERNAL.withDescription("trailer description"); - Metadata trailers = new Metadata(); - interceptedLoggingCall.get().close(status, trailers); - verify(mockLogHelper, never()).logHalfClose( - anyLong(), - AdditionalMatchers.or(ArgumentMatchers.isNull(), anyString()), - AdditionalMatchers.or(ArgumentMatchers.isNull(), anyString()), - AdditionalMatchers.or(ArgumentMatchers.isNull(), anyString()), - any(GrpcLogRecord.EventLogger.class), - anyString()); - capturedListener.onCancel(); - assertThat(Mockito.mockingDetails(mockLogHelper).getInvocations().size()).isEqualTo(6); - } - } } diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/logging/GcpLogSinkTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/logging/GcpLogSinkTest.java index 912d6a08bdb..e02cc6dd4eb 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/logging/GcpLogSinkTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/logging/GcpLogSinkTest.java @@ -169,10 +169,10 @@ public void emptyCustomTags_labelsNotSet() { @SuppressWarnings("unchecked") public void emptyCustomTags_setSourceProject() { Map emptyCustomTags = null; - String destinationProjectId = "DESTINATION_PROJECT"; + String projectId = "PROJECT"; Map expectedLabels = GcpLogSink.getCustomTags(emptyCustomTags, LOCATION_TAGS, - destinationProjectId); - GcpLogSink sink = new GcpLogSink(mockLogging, destinationProjectId, LOCATION_TAGS, + projectId); + GcpLogSink sink = new GcpLogSink(mockLogging, projectId, LOCATION_TAGS, emptyCustomTags, Collections.emptySet()); sink.write(LOG_PROTO); From 6c56c7cfdb28aa8b84027f2165fcf328c11f58c5 Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Fri, 14 Oct 2022 20:41:18 -0700 Subject: [PATCH 2/8] added logic for detecting project id from environment; addressed comments(1) --- .../observability/ObservabilityConfig.java | 27 +- .../ObservabilityConfigImpl.java | 232 ++++++++++-------- .../interceptors/ConfigFilterHelper.java | 11 +- .../ObservabilityConfigImplTest.java | 61 ++--- .../interceptors/ConfigFilterHelperTest.java | 24 +- 5 files changed, 172 insertions(+), 183 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java index fc44988ff84..0489c8b5e3b 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfig.java @@ -54,44 +54,39 @@ public interface ObservabilityConfig { */ @ThreadSafe class LogFilter { - /** Pattern for service/method filter. */ - public final List pattern; - - /** Boolean to indicate all services and methods. */ - public final Boolean matchAll; - /** Set of services. */ public final Set services; /* Set of fullMethodNames. */ public final Set methods; + /** Boolean to indicate all services and methods. */ + public final boolean matchAll; + /** Number of bytes of header to log. */ - public final Integer headerBytes; + public final int headerBytes; /** Number of bytes of message to log. */ - public final Integer messageBytes; + public final int messageBytes; /** Boolean to indicate if services and methods matching pattern needs to be excluded. */ - public final Boolean excludePattern; + public final boolean excludePattern; /** * Object used to represent filter used in configuration. - * @param pattern List of service/method filter - * @param matchAll If true, match all services and methods * @param services Set of services derived from pattern * @param serviceMethods Set of fullMethodNames derived from pattern + * @param matchAll If true, match all services and methods * @param headerBytes Total number of bytes of header to log * @param messageBytes Total number of bytes of message to log * @param excludePattern If true, services and methods matching pattern be excluded */ - public LogFilter(List pattern, Boolean matchAll, Set services, - Set serviceMethods, Integer headerBytes, Integer messageBytes, - Boolean excludePattern) { - this.pattern = pattern; - this.matchAll = matchAll; + public LogFilter(Set services, Set serviceMethods, boolean matchAll, + int headerBytes, int messageBytes, + boolean excludePattern) { this.services = services; this.methods = serviceMethods; + this.matchAll = matchAll; this.headerBytes = headerBytes; this.messageBytes = messageBytes; this.excludePattern = excludePattern; diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java index 5225836a8fd..977ee33eed2 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument; +import com.google.cloud.ServiceOptions; import com.google.common.base.Charsets; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -30,31 +31,32 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Paths; +import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.logging.Level; +import java.util.logging.Logger; import java.util.regex.Pattern; -import java.util.stream.Collectors; /** * gRPC GcpObservability configuration processor. */ final class ObservabilityConfigImpl implements ObservabilityConfig { + private static final Logger logger = Logger + .getLogger(ObservabilityConfigImpl.class.getName()); private static final String CONFIG_ENV_VAR_NAME = "GRPC_GCP_OBSERVABILITY_CONFIG"; private static final String CONFIG_FILE_ENV_VAR_NAME = "GRPC_GCP_OBSERVABILITY_CONFIG_FILE"; - private static final String CONFIG_LOGGING_VAR_NAME = "cloud_logging"; - private static final String CONFIG_MONITORING_VAR_NAME = "cloud_monitoring"; - private static final String CONFIG_TRACING_VAR_NAME = "cloud_tracing"; - private static final String CONFIG_LABELS_VAR_NAME = "labels"; // Tolerance for floating-point comparisons. private static final double EPSILON = 1e-6; private static final Pattern METHOD_NAME_REGEX = - Pattern.compile("^([\\w./]+)/((?:\\w+)|[*])$"); + Pattern.compile("^([*])|((([\\w]+)/((?:\\w+)|[*])))$"); private boolean enableCloudLogging = false; private boolean enableCloudMonitoring = false; private boolean enableCloudTracing = false; private String projectId = null; + private List clientLogFilters; private List serverLogFilters; private Sampler sampler; @@ -85,129 +87,145 @@ void parse(String config) throws IOException { } private void parseConfig(Map config) { - if (config != null) { - projectId = JsonUtil.getString(config, "project_id"); - parseLoggingObject(config, CONFIG_LOGGING_VAR_NAME); - parseMonitoringObject(config, CONFIG_MONITORING_VAR_NAME); - parseTracingObject(config, CONFIG_TRACING_VAR_NAME); - parseCustomTags(config, CONFIG_LABELS_VAR_NAME); - } - } + checkArgument(config != null, "Invalid configuration"); + projectId = fetchProjectId(JsonUtil.getString(config, "project_id")); - private void parseLoggingObject(Map config, String jsonObjectName) { - Map rawCloudLogging = JsonUtil.getObject(config, jsonObjectName); - if (rawCloudLogging != null) { + Map rawCloudLoggingObject = JsonUtil.getObject(config, "cloud_logging"); + if (rawCloudLoggingObject != null) { enableCloudLogging = true; - List rawClientRpcEvents = JsonUtil.getList(rawCloudLogging, "client_rpc_events"); - clientLogFilters = - rawClientRpcEvents != null ? parseRpcEvents(rawClientRpcEvents) : clientLogFilters; - List rawServerRpcEvents = JsonUtil.getList(rawCloudLogging, "server_rpc_events"); - serverLogFilters = - rawServerRpcEvents != null ? parseRpcEvents(rawServerRpcEvents) : serverLogFilters; + ImmutableList.Builder clientFiltersBuilder = new ImmutableList.Builder<>(); + ImmutableList.Builder serverFiltersBuilder = new ImmutableList.Builder<>(); + parseLoggingObject(rawCloudLoggingObject, clientFiltersBuilder, serverFiltersBuilder); + clientLogFilters = clientFiltersBuilder.build(); + serverLogFilters = serverFiltersBuilder.build(); } - } - private void parseMonitoringObject(Map config, String jsonObjectName) { - Map rawCloudMonitoring = JsonUtil.getObject(config, jsonObjectName); - if (rawCloudMonitoring != null) { - if (rawCloudMonitoring.isEmpty()) { - enableCloudMonitoring = true; - } + Map rawCloudMonitoringObject = JsonUtil.getObject(config, "cloud_monitoring"); + if (rawCloudMonitoringObject != null) { + enableCloudMonitoring = true; } - } - private void parseTracingObject(Map config, String jsonObjectName) { - Map rawCloudTracing = JsonUtil.getObject(config, jsonObjectName); - if (rawCloudTracing != null) { + Map rawCloudTracingObject = JsonUtil.getObject(config, "cloud_tracing"); + if (rawCloudTracingObject != null) { enableCloudTracing = true; - this.sampler = Samplers.probabilitySampler(0.0); - if (!rawCloudTracing.isEmpty()) { - Double samplingRate = JsonUtil.getNumberAsDouble(rawCloudTracing, "sampling_rate"); - if (samplingRate != null) { - checkArgument( - samplingRate >= 0.0 && samplingRate <= 1.0, - "'sampling_rate' needs to be between [0.0, 1.0]"); - // Using alwaysSample() instead of probabilitySampler() because according to - // {@link io.opencensus.trace.samplers.ProbabilitySampler#shouldSample} - // there is a (very) small chance of *not* sampling if probability = 1.00. - this.sampler = - 1 - samplingRate < EPSILON - ? Samplers.alwaysSample() - : Samplers.probabilitySampler(samplingRate); - } - } + sampler = parseTracingObject(rawCloudTracingObject); + } + + Map rawCustomTagsObject = JsonUtil.getObject(config, "labels"); + if (rawCustomTagsObject != null) { + customTags = parseCustomTags(rawCustomTagsObject); + } + + if (clientLogFilters == null) { + clientLogFilters = Collections.emptyList(); + } + if (serverLogFilters == null) { + serverLogFilters = Collections.emptyList(); + } + if (customTags == null) { + customTags = Collections.emptyMap(); } } - private void parseCustomTags(Map config, String jsonObjectName) { - Map rawCustomTags = JsonUtil.getObject(config, jsonObjectName); - if (rawCustomTags != null) { - ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); - for (Map.Entry entry: rawCustomTags.entrySet()) { - checkArgument( - entry.getValue() instanceof String, - "'labels' needs to be a map of "); - builder.put(entry.getKey(), (String) entry.getValue()); - } - customTags = builder.build(); + String fetchProjectId(String configProjectId) { + // If project_id is not specified in config, get default GCP project id from the environment + String projectId = configProjectId != null ? configProjectId : getDefaultGcpProjectId(); + checkArgument(projectId != null, "Unable to detect project_id"); + logger.log(Level.INFO, "Found project ID : ", projectId); + return projectId; + } + + String getDefaultGcpProjectId() { + return ServiceOptions.getDefaultProjectId(); + } + + static void parseLoggingObject( + Map rawLoggingConfig, + ImmutableList.Builder clientFilters, + ImmutableList.Builder serverFilters) { + parseRpcEvents(JsonUtil.getList(rawLoggingConfig, "client_rpc_events"), clientFilters); + parseRpcEvents(JsonUtil.getList(rawLoggingConfig, "server_rpc_events"), serverFilters); + } + + static Sampler parseTracingObject(Map rawCloudTracingConfig) { + Sampler defaultSampler = Samplers.probabilitySampler(0.0); + Double samplingRate = JsonUtil.getNumberAsDouble(rawCloudTracingConfig, "sampling_rate"); + if (samplingRate == null) { + return defaultSampler; + } + checkArgument(samplingRate >= 0.0 && samplingRate <= 1.0, + "'sampling_rate' needs to be between [0.0, 1.0]"); + // Using alwaysSample() instead of probabilitySampler() because according to + // {@link io.opencensus.trace.samplers.ProbabilitySampler#shouldSample} + // there is a (very) small chance of *not* sampling if probability = 1.00. + return 1 - samplingRate < EPSILON ? Samplers.alwaysSample() + : Samplers.probabilitySampler(samplingRate); + } + + static Map parseCustomTags(Map rawCustomTags) { + ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); + for (Map.Entry entry: rawCustomTags.entrySet()) { + checkArgument( + entry.getValue() instanceof String, + "'labels' needs to be a map of "); + builder.put(entry.getKey(), (String) entry.getValue()); } + return builder.build(); } - private List parseRpcEvents(List rpcEvents) { + static void parseRpcEvents(List rpcEvents, ImmutableList.Builder filters) { + if (rpcEvents == null) { + return; + } List> jsonRpcEvents = JsonUtil.checkObjectList(rpcEvents); - ImmutableList.Builder rpcEventsListBuilder = - new ImmutableList.Builder<>(); for (Map jsonClientRpcEvent : jsonRpcEvents) { - rpcEventsListBuilder.add(parseJsonLogFilter(jsonClientRpcEvent)); + filters.add(parseJsonLogFilter(jsonClientRpcEvent)); } - return rpcEventsListBuilder.build(); } + static LogFilter parseJsonLogFilter(Map logFilterMap) { + ImmutableSet.Builder servicesSetBuilder = new ImmutableSet.Builder<>(); + ImmutableSet.Builder methodsSetBuilder = new ImmutableSet.Builder<>(); + boolean wildCardFilter = false; - private LogFilter parseJsonLogFilter(Map logFilterMap) { - Boolean exclude = JsonUtil.getBoolean(logFilterMap, "exclude"); - Boolean global = false; - ImmutableList.Builder patternListBuilder = - new ImmutableList.Builder<>(); - ImmutableSet.Builder servicesSetBuilder = - new ImmutableSet.Builder<>(); - ImmutableSet.Builder methodsSetBuilder = - new ImmutableSet.Builder<>(); - List methodsList = JsonUtil.getList(logFilterMap, "methods"); + boolean excludeFilter = + Boolean.TRUE.equals(JsonUtil.getBoolean(logFilterMap, "exclude")); + List methodsList = JsonUtil.getListOfStrings(logFilterMap, "methods"); if (methodsList != null) { - for (Object pattern : methodsList) { - String methodOrServicePattern = String.valueOf(pattern); - if (methodOrServicePattern != null) { - if (methodOrServicePattern.equals("*")) { - if (exclude != null) { - checkArgument( - !exclude, - "cannot have 'exclude' and '*' wildcard in the same filter"); - } - global = true; - } else { - checkArgument( - METHOD_NAME_REGEX.matcher(methodOrServicePattern).matches(), - "invalid service or method string : " + methodOrServicePattern); - if (methodOrServicePattern.endsWith("/*")) { - String service = MethodDescriptor.extractFullServiceName(methodOrServicePattern); - servicesSetBuilder.add(service); - } else { - methodsSetBuilder.add(methodOrServicePattern); - } - } - } + wildCardFilter = extractMethodOrServicePattern( + methodsList, excludeFilter, servicesSetBuilder, methodsSetBuilder); + } + Integer maxHeaderBytes = JsonUtil.getNumberAsInteger(logFilterMap, "max_metadata_bytes"); + Integer maxMessageBytes = JsonUtil.getNumberAsInteger(logFilterMap, "max_message_bytes"); + + return new LogFilter( + servicesSetBuilder.build(), + methodsSetBuilder.build(), + wildCardFilter, + maxHeaderBytes != null ? maxHeaderBytes.intValue() : 0, + maxMessageBytes != null ? maxMessageBytes.intValue() : 0, + excludeFilter); + } + + static boolean extractMethodOrServicePattern(List patternList, boolean exclude, + ImmutableSet.Builder servicesSetBuilder, + ImmutableSet.Builder methodsSetBuilder) { + boolean globalFilter = false; + for (String methodOrServicePattern : patternList) { + checkArgument( + METHOD_NAME_REGEX.matcher(methodOrServicePattern).matches(), + "invalid service or method filter : " + methodOrServicePattern); + if (methodOrServicePattern.equals("*")) { + checkArgument(!exclude, "cannot have 'exclude' and '*' wildcard in the same filter"); + globalFilter = true; + } else if (methodOrServicePattern.endsWith("/*")) { + String service = MethodDescriptor.extractFullServiceName(methodOrServicePattern); + servicesSetBuilder.add(service); + } else { + methodsSetBuilder.add(methodOrServicePattern); } - patternListBuilder.addAll(methodsList.stream() - .filter(element -> element instanceof String) - .map(element -> (String) element) - .collect(Collectors.toList())); } - return new LogFilter(patternListBuilder.build(), - global, servicesSetBuilder.build(), methodsSetBuilder.build(), - JsonUtil.getNumberAsInteger(logFilterMap, "max_metadata_bytes"), - JsonUtil.getNumberAsInteger(logFilterMap, "max_message_bytes"), - exclude); + return globalFilter; } @Override diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java index 3b88a0c81c1..1ba38970cc7 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java @@ -35,8 +35,7 @@ public class ConfigFilterHelper { private final ObservabilityConfig config; - @VisibleForTesting - ConfigFilterHelper(ObservabilityConfig config) { + private ConfigFilterHelper(ObservabilityConfig config) { this.config = config; } @@ -57,14 +56,16 @@ public static ConfigFilterHelper getInstance(ObservabilityConfig config) { * Filters are evaluated in text order, first match is used. * * @param fullMethodName the fully qualified name of the method + * @param client set to true if method being checked is a client method; false otherwise * @return FilterParams object 1. specifies if the corresponding method needs to be logged * (log field will be set to true) 2. values of payload limits retrieved from configuration */ - public FilterParams logRpcMethod(String fullMethodName, Boolean client) { + public FilterParams logRpcMethod(String fullMethodName, boolean client) { FilterParams params = NO_FILTER_PARAMS; int index = checkNotNull(fullMethodName, "fullMethodName").lastIndexOf('/'); String serviceName = fullMethodName.substring(0, index); + List logFilters = client ? config.getClientLogFilters() : config.getServerLogFilters(); @@ -75,8 +76,8 @@ public FilterParams logRpcMethod(String fullMethodName, Boolean client) { if (logFilter.excludePattern) { return params; } - int currentHeaderBytes = logFilter.headerBytes != null ? logFilter.headerBytes : 0; - int currentMessageBytes = logFilter.messageBytes != null ? logFilter.messageBytes : 0; + int currentHeaderBytes = logFilter.headerBytes; + int currentMessageBytes = logFilter.messageBytes; return FilterParams.create(true, currentHeaderBytes, currentMessageBytes); } } diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java index 3c624ab8cc8..59b6caf343d 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java @@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -30,7 +29,6 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Paths; -import java.util.Arrays; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -206,11 +204,10 @@ public void emptyConfig() throws IOException { assertFalse(observabilityConfig.isEnableCloudLogging()); assertFalse(observabilityConfig.isEnableCloudMonitoring()); assertFalse(observabilityConfig.isEnableCloudTracing()); - assertNull(observabilityConfig.getProjectId()); - assertThat(observabilityConfig.getClientLogFilters()).isNull(); - assertThat(observabilityConfig.getServerLogFilters()).isNull(); + assertThat(observabilityConfig.getClientLogFilters()).isEmpty(); + assertThat(observabilityConfig.getServerLogFilters()).isEmpty(); assertThat(observabilityConfig.getSampler()).isNull(); - assertThat(observabilityConfig.getCustomTags()).isNull(); + assertThat(observabilityConfig.getCustomTags()).isEmpty(); } @Test @@ -231,11 +228,10 @@ public void disableObservability() throws IOException { assertFalse(observabilityConfig.isEnableCloudLogging()); assertFalse(observabilityConfig.isEnableCloudMonitoring()); assertFalse(observabilityConfig.isEnableCloudTracing()); - assertNull(observabilityConfig.getProjectId()); - assertThat(observabilityConfig.getClientLogFilters()).isNull(); - assertThat(observabilityConfig.getServerLogFilters()).isNull(); + assertThat(observabilityConfig.getClientLogFilters()).isEmpty(); + assertThat(observabilityConfig.getServerLogFilters()).isEmpty(); assertThat(observabilityConfig.getSampler()).isNull(); - assertThat(observabilityConfig.getCustomTags()).isNull(); + assertThat(observabilityConfig.getCustomTags()).isEmpty(); } @Test @@ -253,20 +249,18 @@ public void logFilters() throws IOException { List clientLogFilters = observabilityConfig.getClientLogFilters(); assertThat(clientLogFilters).hasSize(1); - assertThat(clientLogFilters.get(0).pattern).isEqualTo(Arrays.asList("*")); assertThat(clientLogFilters.get(0).headerBytes).isEqualTo(4096); - assertThat(clientLogFilters.get(0).messageBytes).isNull(); - assertThat(clientLogFilters.get(0).excludePattern).isNull(); + assertThat(clientLogFilters.get(0).messageBytes).isEqualTo(0); + assertThat(clientLogFilters.get(0).excludePattern).isFalse(); assertThat(clientLogFilters.get(0).matchAll).isTrue(); assertThat(clientLogFilters.get(0).services).isEmpty(); assertThat(clientLogFilters.get(0).methods).isEmpty(); List serverLogFilters = observabilityConfig.getServerLogFilters(); assertThat(serverLogFilters).hasSize(1); - assertThat(serverLogFilters.get(0).pattern).isEqualTo(Arrays.asList("*")); assertThat(serverLogFilters.get(0).headerBytes).isEqualTo(32); assertThat(serverLogFilters.get(0).messageBytes).isEqualTo(64); - assertThat(serverLogFilters.get(0).excludePattern).isNull(); + assertThat(serverLogFilters.get(0).excludePattern).isFalse(); assertThat(serverLogFilters.get(0).matchAll).isTrue(); assertThat(serverLogFilters.get(0).services).isEmpty(); assertThat(serverLogFilters.get(0).methods).isEmpty(); @@ -279,18 +273,15 @@ public void setClientLogFilters() throws IOException { assertThat(observabilityConfig.getProjectId()).isEqualTo("grpc-testing"); List logFilterList = observabilityConfig.getClientLogFilters(); assertThat(logFilterList).hasSize(2); - assertThat(logFilterList.get(0).pattern).isEqualTo(Arrays.asList("*")); assertThat(logFilterList.get(0).headerBytes).isEqualTo(4096); assertThat(logFilterList.get(0).messageBytes).isEqualTo(2048); - assertThat(logFilterList.get(0).excludePattern).isNull(); + assertThat(logFilterList.get(0).excludePattern).isFalse(); assertThat(logFilterList.get(0).matchAll).isTrue(); assertThat(logFilterList.get(0).services).isEmpty(); assertThat(logFilterList.get(0).methods).isEmpty(); - assertThat(logFilterList.get(1).pattern) - .isEqualTo(Arrays.asList("service1/Method2", "Service2/*")); - assertThat(logFilterList.get(1).headerBytes).isNull(); - assertThat(logFilterList.get(1).messageBytes).isNull(); + assertThat(logFilterList.get(1).headerBytes).isEqualTo(0); + assertThat(logFilterList.get(1).messageBytes).isEqualTo(0); assertThat(logFilterList.get(1).excludePattern).isTrue(); assertThat(logFilterList.get(1).matchAll).isFalse(); assertThat(logFilterList.get(1).services).isEqualTo(Collections.singleton("Service2")); @@ -306,11 +297,9 @@ public void setServerLogFilters() throws IOException { assertTrue(observabilityConfig.isEnableCloudLogging()); List logFilterList = observabilityConfig.getServerLogFilters(); assertThat(logFilterList).hasSize(2); - assertThat(logFilterList.get(0).pattern) - .isEqualTo(Arrays.asList("service1/method4", "service2/method234")); assertThat(logFilterList.get(0).headerBytes).isEqualTo(32); assertThat(logFilterList.get(0).messageBytes).isEqualTo(64); - assertThat(logFilterList.get(0).excludePattern).isNull(); + assertThat(logFilterList.get(0).excludePattern).isFalse(); assertThat(logFilterList.get(0).matchAll).isFalse(); assertThat(logFilterList.get(0).services).isEmpty(); assertThat(logFilterList.get(0).methods) @@ -318,10 +307,8 @@ public void setServerLogFilters() throws IOException { Set expectedServices = Stream.of("service4", "Service2") .collect(Collectors.toCollection(HashSet::new)); - assertThat(logFilterList.get(1).pattern) - .isEqualTo(Arrays.asList("service4/*", "Service2/*")); - assertThat(logFilterList.get(1).headerBytes).isNull(); - assertThat(logFilterList.get(1).messageBytes).isNull(); + assertThat(logFilterList.get(1).headerBytes).isEqualTo(0); + assertThat(logFilterList.get(1).messageBytes).isEqualTo(0); assertThat(logFilterList.get(1).excludePattern).isTrue(); assertThat(logFilterList.get(1).matchAll).isFalse(); assertThat(logFilterList.get(1).services).isEqualTo(expectedServices); @@ -405,27 +392,21 @@ public void configFileLogFilters() throws Exception { assertThat(observabilityConfig.getProjectId()).isEqualTo("grpc-testing"); List logFilters = observabilityConfig.getClientLogFilters(); assertThat(logFilters).hasSize(2); - assertThat(logFilters.get(0).pattern).isEqualTo(Arrays.asList("*")); assertThat(logFilters.get(0).headerBytes).isEqualTo(4096); assertThat(logFilters.get(0).messageBytes).isEqualTo(2048); - assertThat(logFilters.get(1).pattern) - .isEqualTo(Arrays.asList("service1/Method2", "Service2/*")); - assertThat(logFilters.get(1).headerBytes).isNull(); - assertThat(logFilters.get(1).messageBytes).isNull(); + assertThat(logFilters.get(1).headerBytes).isEqualTo(0); + assertThat(logFilters.get(1).messageBytes).isEqualTo(0); assertThat(logFilters).hasSize(2); - assertThat(logFilters.get(0).pattern).isEqualTo(Arrays.asList("*")); assertThat(logFilters.get(0).headerBytes).isEqualTo(4096); assertThat(logFilters.get(0).messageBytes).isEqualTo(2048); - assertThat(logFilters.get(0).excludePattern).isNull(); + assertThat(logFilters.get(0).excludePattern).isFalse(); assertThat(logFilters.get(0).matchAll).isTrue(); assertThat(logFilters.get(0).services).isEmpty(); assertThat(logFilters.get(0).methods).isEmpty(); - assertThat(logFilters.get(1).pattern) - .isEqualTo(Arrays.asList("service1/Method2", "Service2/*")); - assertThat(logFilters.get(1).headerBytes).isNull(); - assertThat(logFilters.get(1).messageBytes).isNull(); + assertThat(logFilters.get(1).headerBytes).isEqualTo(0); + assertThat(logFilters.get(1).messageBytes).isEqualTo(0); assertThat(logFilters.get(1).excludePattern).isTrue(); assertThat(logFilters.get(1).matchAll).isFalse(); assertThat(logFilters.get(1).services).isEqualTo(Collections.singleton("Service2")); @@ -473,7 +454,7 @@ public void logFilterInvalidMethod() throws IOException { fail("exception expected!"); } catch (IllegalArgumentException iae) { assertThat(iae.getMessage()).contains( - "invalid service or method string"); + "invalid service or method filter"); } } } diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelperTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelperTest.java index 5076a6b1646..971e6070777 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelperTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelperTest.java @@ -24,7 +24,6 @@ import io.grpc.gcp.observability.ObservabilityConfig; import io.grpc.gcp.observability.ObservabilityConfig.LogFilter; import io.grpc.gcp.observability.interceptors.ConfigFilterHelper.FilterParams; -import java.util.Arrays; import java.util.Collections; import java.util.List; import org.junit.Before; @@ -33,19 +32,16 @@ public class ConfigFilterHelperTest { private static final ImmutableList configLogFilters = ImmutableList.of( - new LogFilter(Collections.singletonList("service1/Method2"), false, - Collections.emptySet(), Collections.singleton("service1/Method2"), + new LogFilter(Collections.emptySet(), Collections.singleton("service1/Method2"), false, 1024, 1024, false), new LogFilter( - Arrays.asList("service2/*, service4/method2"), false, - Collections.singleton("service2"), Collections.singleton("service4/method2"), + Collections.singleton("service2"), Collections.singleton("service4/method2"), false, 2048, 1024, false), new LogFilter( - Arrays.asList("service2/*, service4/method3"), false, - Collections.singleton("service2"), Collections.singleton("service4/method3"), + Collections.singleton("service2"), Collections.singleton("service4/method3"), false, 2048, 1024, true), new LogFilter( - Collections.singletonList("*"), true, Collections.emptySet(), Collections.emptySet(), + Collections.emptySet(), Collections.emptySet(), true, 128, 128, false)); private ObservabilityConfig mockConfig; @@ -54,7 +50,7 @@ public class ConfigFilterHelperTest { @Before public void setup() { mockConfig = mock(ObservabilityConfig.class); - configFilterHelper = new ConfigFilterHelper(mockConfig); + configFilterHelper = ConfigFilterHelper.getInstance(mockConfig); } @Test @@ -85,8 +81,8 @@ public void checkMethodAlwaysLogged() { List sampleLogFilters = ImmutableList.of( new LogFilter( - Collections.singletonList("*"), true, Collections.emptySet(), - Collections.emptySet(), 4096, 4096, false)); + Collections.emptySet(), Collections.emptySet(), true, + 4096, 4096, false)); when(mockConfig.getClientLogFilters()).thenReturn(sampleLogFilters); when(mockConfig.getServerLogFilters()).thenReturn(sampleLogFilters); @@ -104,12 +100,10 @@ public void checkMethodAlwaysLogged() { public void checkMethodNotToBeLogged() { List sampleLogFilters = ImmutableList.of( - new LogFilter(Collections.singletonList("service2/*"), false, - Collections.emptySet(), Collections.singleton("service2/*"), + new LogFilter(Collections.emptySet(), Collections.singleton("service2/*"), false, 1024, 1024, true), new LogFilter( - Collections.singletonList("service2/Method1"), false, - Collections.singleton("service2/Method1"), Collections.emptySet(), + Collections.singleton("service2/Method1"), Collections.emptySet(), false, 2048, 1024, false)); when(mockConfig.getClientLogFilters()).thenReturn(sampleLogFilters); when(mockConfig.getServerLogFilters()).thenReturn(sampleLogFilters); From 4a4b5595b7f4b9bc15f676b4f4fe074ba9e78d8b Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Sun, 16 Oct 2022 11:15:09 -0700 Subject: [PATCH 3/8] updated logic for empty config --- .../ObservabilityConfigImpl.java | 8 ++++- .../ObservabilityConfigImplTest.java | 30 ++++++++++--------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java index 977ee33eed2..07a6cfe50c6 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java @@ -88,6 +88,12 @@ void parse(String config) throws IOException { private void parseConfig(Map config) { checkArgument(config != null, "Invalid configuration"); + if (config.isEmpty()) { + clientLogFilters = Collections.emptyList(); + serverLogFilters = Collections.emptyList(); + customTags = Collections.emptyMap(); + return; + } projectId = fetchProjectId(JsonUtil.getString(config, "project_id")); Map rawCloudLoggingObject = JsonUtil.getObject(config, "cloud_logging"); @@ -131,7 +137,7 @@ String fetchProjectId(String configProjectId) { // If project_id is not specified in config, get default GCP project id from the environment String projectId = configProjectId != null ? configProjectId : getDefaultGcpProjectId(); checkArgument(projectId != null, "Unable to detect project_id"); - logger.log(Level.INFO, "Found project ID : ", projectId); + logger.log(Level.FINE, "Found project ID : ", projectId); return projectId; } diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java index 59b6caf343d..5bf286f392a 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java @@ -79,6 +79,7 @@ public class ObservabilityConfigImplTest { + "}"; private static final String SERVER_LOG_FILTERS = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_logging\": {\n" + " \"server_rpc_events\": [{\n" + " \"methods\": [\"service1/method4\", \"service2/method234\"],\n" @@ -94,48 +95,57 @@ public class ObservabilityConfigImplTest { + "}"; private static final String PROJECT_ID = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_logging\": {},\n" + " \"project_id\": \"grpc-testing\"\n" + "}"; - private static final String DISABLE_OBSERVABILITY = "{}"; + private static final String EMPTY_CONFIG = "{}"; private static final String ENABLE_CLOUD_MONITORING_AND_TRACING = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_monitoring\": {},\n" + " \"cloud_tracing\": {}\n" + "}"; private static final String ENABLE_CLOUD_MONITORING = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_monitoring\": {}\n" + "}"; private static final String ENABLE_CLOUD_TRACING = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_tracing\": {}\n" + "}"; private static final String TRACING_ALWAYS_SAMPLER = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_tracing\": {\n" + " \"sampling_rate\": 1.00\n" + " }\n" + "}"; private static final String TRACING_NEVER_SAMPLER = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_tracing\": {\n" + " \"sampling_rate\": 0.00\n" + " }\n" + "}"; private static final String TRACING_PROBABILISTIC_SAMPLER = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_tracing\": {\n" + " \"sampling_rate\": 0.75\n" + " }\n" + "}"; private static final String TRACING_DEFAULT_SAMPLER = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_tracing\": {}\n" + "}"; private static final String GLOBAL_TRACING_BAD_PROBABILISTIC_SAMPLER = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_tracing\": {\n" + " \"sampling_rate\": -0.75\n" + " }\n" @@ -152,6 +162,7 @@ public class ObservabilityConfigImplTest { private static final String BAD_CUSTOM_TAGS = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_monitoring\": {},\n" + " \"labels\": {\n" + " \"SOURCE_VERSION\" : \"J2e1Cf\",\n" @@ -162,6 +173,7 @@ public class ObservabilityConfigImplTest { private static final String LOG_FILTER_GLOBAL_EXCLUDE = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_logging\": {\n" + " \"client_rpc_events\": [{\n" + " \"methods\": [\"service1/Method2\", \"*\"],\n" @@ -175,6 +187,7 @@ public class ObservabilityConfigImplTest { private static final String LOG_FILTER_INVALID_METHOD = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_logging\": {\n" + " \"client_rpc_events\": [{\n" + " \"methods\": [\"s*&%ervice1/Method2\", \"*\"],\n" @@ -200,13 +213,14 @@ public void nullConfig() throws IOException { @Test public void emptyConfig() throws IOException { - observabilityConfig.parse("{}"); + observabilityConfig.parse(EMPTY_CONFIG); assertFalse(observabilityConfig.isEnableCloudLogging()); assertFalse(observabilityConfig.isEnableCloudMonitoring()); assertFalse(observabilityConfig.isEnableCloudTracing()); assertThat(observabilityConfig.getClientLogFilters()).isEmpty(); assertThat(observabilityConfig.getServerLogFilters()).isEmpty(); assertThat(observabilityConfig.getSampler()).isNull(); + assertThat(observabilityConfig.getProjectId()).isNull(); assertThat(observabilityConfig.getCustomTags()).isEmpty(); } @@ -222,18 +236,6 @@ public void emptyConfigFile() throws IOException { } } - @Test - public void disableObservability() throws IOException { - observabilityConfig.parse(DISABLE_OBSERVABILITY); - assertFalse(observabilityConfig.isEnableCloudLogging()); - assertFalse(observabilityConfig.isEnableCloudMonitoring()); - assertFalse(observabilityConfig.isEnableCloudTracing()); - assertThat(observabilityConfig.getClientLogFilters()).isEmpty(); - assertThat(observabilityConfig.getServerLogFilters()).isEmpty(); - assertThat(observabilityConfig.getSampler()).isNull(); - assertThat(observabilityConfig.getCustomTags()).isEmpty(); - } - @Test public void setProjectId() throws IOException { observabilityConfig.parse(PROJECT_ID); From 2051d705caffc353803398418266bbe7d2ab2a2a Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Sun, 16 Oct 2022 11:27:09 -0700 Subject: [PATCH 4/8] fixed config test --- .../io/grpc/gcp/observability/ObservabilityConfigImplTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java index 5bf286f392a..69e77256308 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java @@ -152,6 +152,7 @@ public class ObservabilityConfigImplTest { + "}"; private static final String CUSTOM_TAGS = "{\n" + + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_logging\": {},\n" + " \"labels\": {\n" + " \"SOURCE_VERSION\" : \"J2e1Cf\",\n" From cd40934649663c5c14bf1ff33b9841165e1e5676 Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Mon, 17 Oct 2022 09:05:47 -0700 Subject: [PATCH 5/8] updated trace config variable name to --- .../observability/ObservabilityConfigImpl.java | 14 +++++++------- .../interceptors/ConfigFilterHelper.java | 1 + .../ObservabilityConfigImplTest.java | 18 +++++++++--------- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java index 07a6cfe50c6..a1d0efa6d3e 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java @@ -23,7 +23,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import io.grpc.MethodDescriptor; import io.grpc.internal.JsonParser; import io.grpc.internal.JsonUtil; import io.opencensus.trace.Sampler; @@ -36,6 +35,7 @@ import java.util.Map; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.regex.Matcher; import java.util.regex.Pattern; /** @@ -111,7 +111,7 @@ private void parseConfig(Map config) { enableCloudMonitoring = true; } - Map rawCloudTracingObject = JsonUtil.getObject(config, "cloud_tracing"); + Map rawCloudTracingObject = JsonUtil.getObject(config, "cloud_trace"); if (rawCloudTracingObject != null) { enableCloudTracing = true; sampler = parseTracingObject(rawCloudTracingObject); @@ -218,14 +218,14 @@ static boolean extractMethodOrServicePattern(List patternList, boolean e ImmutableSet.Builder methodsSetBuilder) { boolean globalFilter = false; for (String methodOrServicePattern : patternList) { + Matcher matcher = METHOD_NAME_REGEX.matcher(methodOrServicePattern); checkArgument( - METHOD_NAME_REGEX.matcher(methodOrServicePattern).matches(), - "invalid service or method filter : " + methodOrServicePattern); - if (methodOrServicePattern.equals("*")) { + matcher.matches(), "invalid service or method filter : " + methodOrServicePattern); + if ("*".equals(methodOrServicePattern)) { checkArgument(!exclude, "cannot have 'exclude' and '*' wildcard in the same filter"); globalFilter = true; - } else if (methodOrServicePattern.endsWith("/*")) { - String service = MethodDescriptor.extractFullServiceName(methodOrServicePattern); + } else if ("*".equals(matcher.group(5))) { + String service = matcher.group(4); servicesSetBuilder.add(service); } else { methodsSetBuilder.add(methodOrServicePattern); diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java index 1ba38970cc7..9b05634dbfe 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/ConfigFilterHelper.java @@ -69,6 +69,7 @@ public FilterParams logRpcMethod(String fullMethodName, boolean client) { List logFilters = client ? config.getClientLogFilters() : config.getServerLogFilters(); + // TODO (dnvindhya): Optimize by caching results for fullMethodName. for (LogFilter logFilter : logFilters) { if (logFilter.matchAll || logFilter.services.contains(serviceName) diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java index 69e77256308..1761c641434 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityConfigImplTest.java @@ -105,7 +105,7 @@ public class ObservabilityConfigImplTest { private static final String ENABLE_CLOUD_MONITORING_AND_TRACING = "{\n" + " \"project_id\": \"grpc-testing\",\n" + " \"cloud_monitoring\": {},\n" - + " \"cloud_tracing\": {}\n" + + " \"cloud_trace\": {}\n" + "}"; private static final String ENABLE_CLOUD_MONITORING = "{\n" @@ -113,40 +113,40 @@ public class ObservabilityConfigImplTest { + " \"cloud_monitoring\": {}\n" + "}"; - private static final String ENABLE_CLOUD_TRACING = "{\n" + private static final String ENABLE_CLOUD_TRACE = "{\n" + " \"project_id\": \"grpc-testing\",\n" - + " \"cloud_tracing\": {}\n" + + " \"cloud_trace\": {}\n" + "}"; private static final String TRACING_ALWAYS_SAMPLER = "{\n" + " \"project_id\": \"grpc-testing\",\n" - + " \"cloud_tracing\": {\n" + + " \"cloud_trace\": {\n" + " \"sampling_rate\": 1.00\n" + " }\n" + "}"; private static final String TRACING_NEVER_SAMPLER = "{\n" + " \"project_id\": \"grpc-testing\",\n" - + " \"cloud_tracing\": {\n" + + " \"cloud_trace\": {\n" + " \"sampling_rate\": 0.00\n" + " }\n" + "}"; private static final String TRACING_PROBABILISTIC_SAMPLER = "{\n" + " \"project_id\": \"grpc-testing\",\n" - + " \"cloud_tracing\": {\n" + + " \"cloud_trace\": {\n" + " \"sampling_rate\": 0.75\n" + " }\n" + "}"; private static final String TRACING_DEFAULT_SAMPLER = "{\n" + " \"project_id\": \"grpc-testing\",\n" - + " \"cloud_tracing\": {}\n" + + " \"cloud_trace\": {}\n" + "}"; private static final String GLOBAL_TRACING_BAD_PROBABILISTIC_SAMPLER = "{\n" + " \"project_id\": \"grpc-testing\",\n" - + " \"cloud_tracing\": {\n" + + " \"cloud_trace\": {\n" + " \"sampling_rate\": -0.75\n" + " }\n" + "}"; @@ -326,7 +326,7 @@ public void enableCloudMonitoring() throws IOException { @Test public void enableCloudTracing() throws IOException { - observabilityConfig.parse(ENABLE_CLOUD_TRACING); + observabilityConfig.parse(ENABLE_CLOUD_TRACE); assertTrue(observabilityConfig.isEnableCloudTracing()); } From fcda3cb34cd6b8f036a271285f47c058e09e0867 Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Mon, 17 Oct 2022 11:51:58 -0700 Subject: [PATCH 6/8] added unit test to use mock GcpLogSink; addressed comments(2) --- .../{LoggingTestHelper.java => ObservabilityTestHelper.java} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename gcp-observability/src/test/java/io/grpc/gcp/observability/{LoggingTestHelper.java => ObservabilityTestHelper.java} (100%) diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTestHelper.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityTestHelper.java similarity index 100% rename from gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTestHelper.java rename to gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityTestHelper.java From 2c5fbfa9459da26189351685050046f77fc2f77c Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Mon, 17 Oct 2022 11:52:21 -0700 Subject: [PATCH 7/8] added unit test to use mock GcpLogSink; addressed comments(2) --- .../ObservabilityConfigImpl.java | 2 +- .../grpc/gcp/observability/LoggingTest.java | 74 ++++++++++++++++++- .../grpc/gcp/observability/MetricsTest.java | 4 +- .../ObservabilityTestHelper.java | 2 +- .../io/grpc/gcp/observability/TracesTest.java | 4 +- 5 files changed, 76 insertions(+), 10 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java index a1d0efa6d3e..895a7174554 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java @@ -137,7 +137,7 @@ String fetchProjectId(String configProjectId) { // If project_id is not specified in config, get default GCP project id from the environment String projectId = configProjectId != null ? configProjectId : getDefaultGcpProjectId(); checkArgument(projectId != null, "Unable to detect project_id"); - logger.log(Level.FINE, "Found project ID : ", projectId); + logger.log(Level.FINEST, "Found project ID : ", projectId); return projectId; } diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java index 913e2b75113..35e77761d21 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java @@ -21,6 +21,8 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -36,6 +38,8 @@ import io.grpc.gcp.observability.interceptors.LogHelper; import io.grpc.gcp.observability.logging.GcpLogSink; import io.grpc.gcp.observability.logging.Sink; +import io.grpc.internal.TimeProvider; +import io.grpc.observabilitylog.v1.GrpcLogRecord; import io.grpc.testing.GrpcCleanupRule; import io.grpc.testing.protobuf.SimpleServiceGrpc; import java.io.IOException; @@ -46,6 +50,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.ArgumentCaptor; import org.mockito.Mockito; @RunWith(JUnit4.class) @@ -95,6 +100,13 @@ public void clientServer_interceptorCalled_logNever() throws Exception { ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); } + @Test + public void clientServer_interceptorCalled_logEvents_usingMockSink() throws Exception { + Class runnable = + classLoader.loadClass(StaticTestingClassLogEventsUsingMockSink.class.getName()); + ((Runnable) runnable.getDeclaredConstructor().newInstance()).run(); + } + // UsedReflectively public static final class StaticTestingClassEndtoEndLogging implements Runnable { @@ -123,7 +135,7 @@ public void run() { sink, config, channelInterceptorFactory, serverInterceptorFactory)) { Server server = ServerBuilder.forPort(0) - .addService(new LoggingTestHelper.SimpleServiceImpl()) + .addService(new ObservabilityTestHelper.SimpleServiceImpl()) .build() .start(); int port = cleanupRule.register(server).getPort(); @@ -131,7 +143,7 @@ public void run() { SimpleServiceGrpc.newBlockingStub( cleanupRule.register( ManagedChannelBuilder.forAddress("localhost", port).usePlaintext().build())); - assertThat(LoggingTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) + assertThat(ObservabilityTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) .isEqualTo("Hello buddy"); assertThat(Mockito.mockingDetails(spyLogHelper).getInvocations().size()).isGreaterThan(11); } catch (IOException e) { @@ -165,7 +177,7 @@ public void run() { mockSink, config, channelInterceptorFactory, serverInterceptorFactory)) { Server server = ServerBuilder.forPort(0) - .addService(new LoggingTestHelper.SimpleServiceImpl()) + .addService(new ObservabilityTestHelper.SimpleServiceImpl()) .build() .start(); int port = cleanupRule.register(server).getPort(); @@ -173,7 +185,7 @@ public void run() { SimpleServiceGrpc.newBlockingStub( cleanupRule.register( ManagedChannelBuilder.forAddress("localhost", port).usePlaintext().build())); - assertThat(LoggingTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) + assertThat(ObservabilityTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) .isEqualTo("Hello buddy"); verifyNoInteractions(spyLogHelper); verifyNoInteractions(mockSink); @@ -182,4 +194,58 @@ public void run() { } } } + + public static final class StaticTestingClassLogEventsUsingMockSink implements Runnable { + + @Override + public void run() { + Sink mockSink = mock(GcpLogSink.class); + ObservabilityConfig config = mock(ObservabilityConfig.class); + LogHelper spyLogHelper = spy(new LogHelper(mockSink, TimeProvider.SYSTEM_TIME_PROVIDER)); + ConfigFilterHelper mockFilterHelper2 = mock(ConfigFilterHelper.class); + InternalLoggingChannelInterceptor.Factory channelInterceptorFactory = + new InternalLoggingChannelInterceptor.FactoryImpl(spyLogHelper, mockFilterHelper2); + InternalLoggingServerInterceptor.Factory serverInterceptorFactory = + new InternalLoggingServerInterceptor.FactoryImpl(spyLogHelper, mockFilterHelper2); + + when(config.isEnableCloudLogging()).thenReturn(true); + FilterParams logAlwaysFilterParams = FilterParams.create(true, 0, 0); + when(mockFilterHelper2.logRpcMethod(anyString(), eq(true))) + .thenReturn(logAlwaysFilterParams); + when(mockFilterHelper2.logRpcMethod(anyString(), eq(false))) + .thenReturn(logAlwaysFilterParams); + + try (GcpObservability observability = + GcpObservability.grpcInit( + mockSink, config, channelInterceptorFactory, serverInterceptorFactory)) { + Server server = + ServerBuilder.forPort(0) + .addService(new ObservabilityTestHelper.SimpleServiceImpl()) + .build() + .start(); + int port = cleanupRule.register(server).getPort(); + SimpleServiceGrpc.SimpleServiceBlockingStub stub = + SimpleServiceGrpc.newBlockingStub( + cleanupRule.register( + ManagedChannelBuilder.forAddress("localhost", port).usePlaintext().build())); + assertThat(ObservabilityTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) + .isEqualTo("Hello buddy"); + // Total number of calls should have been 14 (6 from client and 6 from server) + // Since cancel is not invoked, it will be 12. + // Request message(Total count:2 (1 from client and 1 from server) and Response + // message(count:2) + // events are not in the event_types list, i.e 14 - 2(cancel) - 2(req_msg) - 2(resp_msg) + // = 8 + assertThat(Mockito.mockingDetails(mockSink).getInvocations().size()).isEqualTo(12); + ArgumentCaptor captor = ArgumentCaptor.forClass(GrpcLogRecord.class); + verify(mockSink, times(12)).write(captor.capture()); + for (GrpcLogRecord record : captor.getAllValues()) { + assertThat(record.getEventType()).isInstanceOf(GrpcLogRecord.EventType.class); + assertThat(record.getEventLogger()).isInstanceOf(GrpcLogRecord.EventLogger.class); + } + } catch (IOException e) { + throw new AssertionError("Exception while testing logging using mock sink", e); + } + } + } } diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/MetricsTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/MetricsTest.java index 645a4027bf7..046799cc9d2 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/MetricsTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/MetricsTest.java @@ -107,7 +107,7 @@ public void run() { Server server = ServerBuilder.forPort(0) - .addService(new LoggingTestHelper.SimpleServiceImpl()) + .addService(new ObservabilityTestHelper.SimpleServiceImpl()) .build() .start(); int port = cleanupRule.register(server).getPort(); @@ -115,7 +115,7 @@ public void run() { SimpleServiceGrpc.newBlockingStub( cleanupRule.register( ManagedChannelBuilder.forAddress("localhost", port).usePlaintext().build())); - assertThat(LoggingTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) + assertThat(ObservabilityTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) .isEqualTo("Hello buddy"); // Adding sleep to ensure metrics are exported before querying cloud monitoring backend TimeUnit.SECONDS.sleep(40); diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityTestHelper.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityTestHelper.java index 529ec2503fd..ebb73ec76a1 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityTestHelper.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/ObservabilityTestHelper.java @@ -21,7 +21,7 @@ import io.grpc.testing.protobuf.SimpleResponse; import io.grpc.testing.protobuf.SimpleServiceGrpc; -public class LoggingTestHelper { +public class ObservabilityTestHelper { static String makeUnaryRpcViaClientStub( String requestMessage, SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub) { diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java index 534f1236850..ae7aa63befc 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/TracesTest.java @@ -110,7 +110,7 @@ public void run() { Server server = ServerBuilder.forPort(0) - .addService(new LoggingTestHelper.SimpleServiceImpl()) + .addService(new ObservabilityTestHelper.SimpleServiceImpl()) .build() .start(); int port = cleanupRule.register(server).getPort(); @@ -118,7 +118,7 @@ public void run() { SimpleServiceGrpc.newBlockingStub( cleanupRule.register( ManagedChannelBuilder.forAddress("localhost", port).usePlaintext().build())); - assertThat(LoggingTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) + assertThat(ObservabilityTestHelper.makeUnaryRpcViaClientStub("buddy", stub)) .isEqualTo("Hello buddy"); // Adding sleep to ensure traces are exported before querying cloud tracing backend TimeUnit.SECONDS.sleep(10); From 7c24dafdc84296350d8a35849091b9cfd41b832f Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Tue, 18 Oct 2022 11:49:45 -0700 Subject: [PATCH 8/8] resolved conflicts from merging logging proto changes --- .../observability/ObservabilityConfigImpl.java | 16 ++++++++-------- .../io/grpc/gcp/observability/LoggingTest.java | 7 +++---- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java index 895a7174554..34bf0e19e63 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java @@ -133,7 +133,7 @@ private void parseConfig(Map config) { } } - String fetchProjectId(String configProjectId) { + private static String fetchProjectId(String configProjectId) { // If project_id is not specified in config, get default GCP project id from the environment String projectId = configProjectId != null ? configProjectId : getDefaultGcpProjectId(); checkArgument(projectId != null, "Unable to detect project_id"); @@ -141,11 +141,11 @@ String fetchProjectId(String configProjectId) { return projectId; } - String getDefaultGcpProjectId() { + private static String getDefaultGcpProjectId() { return ServiceOptions.getDefaultProjectId(); } - static void parseLoggingObject( + private static void parseLoggingObject( Map rawLoggingConfig, ImmutableList.Builder clientFilters, ImmutableList.Builder serverFilters) { @@ -153,7 +153,7 @@ static void parseLoggingObject( parseRpcEvents(JsonUtil.getList(rawLoggingConfig, "server_rpc_events"), serverFilters); } - static Sampler parseTracingObject(Map rawCloudTracingConfig) { + private static Sampler parseTracingObject(Map rawCloudTracingConfig) { Sampler defaultSampler = Samplers.probabilitySampler(0.0); Double samplingRate = JsonUtil.getNumberAsDouble(rawCloudTracingConfig, "sampling_rate"); if (samplingRate == null) { @@ -168,7 +168,7 @@ static Sampler parseTracingObject(Map rawCloudTracingConfig) { : Samplers.probabilitySampler(samplingRate); } - static Map parseCustomTags(Map rawCustomTags) { + private static Map parseCustomTags(Map rawCustomTags) { ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); for (Map.Entry entry: rawCustomTags.entrySet()) { checkArgument( @@ -179,7 +179,7 @@ static Map parseCustomTags(Map rawCustomTags) { return builder.build(); } - static void parseRpcEvents(List rpcEvents, ImmutableList.Builder filters) { + private static void parseRpcEvents(List rpcEvents, ImmutableList.Builder filters) { if (rpcEvents == null) { return; } @@ -189,7 +189,7 @@ static void parseRpcEvents(List rpcEvents, ImmutableList.Builder f } } - static LogFilter parseJsonLogFilter(Map logFilterMap) { + private static LogFilter parseJsonLogFilter(Map logFilterMap) { ImmutableSet.Builder servicesSetBuilder = new ImmutableSet.Builder<>(); ImmutableSet.Builder methodsSetBuilder = new ImmutableSet.Builder<>(); boolean wildCardFilter = false; @@ -213,7 +213,7 @@ static LogFilter parseJsonLogFilter(Map logFilterMap) { excludeFilter); } - static boolean extractMethodOrServicePattern(List patternList, boolean exclude, + private static boolean extractMethodOrServicePattern(List patternList, boolean exclude, ImmutableSet.Builder servicesSetBuilder, ImmutableSet.Builder methodsSetBuilder) { boolean globalFilter = false; diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java index 35e77761d21..992ccc5dbf5 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/LoggingTest.java @@ -38,7 +38,6 @@ import io.grpc.gcp.observability.interceptors.LogHelper; import io.grpc.gcp.observability.logging.GcpLogSink; import io.grpc.gcp.observability.logging.Sink; -import io.grpc.internal.TimeProvider; import io.grpc.observabilitylog.v1.GrpcLogRecord; import io.grpc.testing.GrpcCleanupRule; import io.grpc.testing.protobuf.SimpleServiceGrpc; @@ -201,7 +200,7 @@ public static final class StaticTestingClassLogEventsUsingMockSink implements Ru public void run() { Sink mockSink = mock(GcpLogSink.class); ObservabilityConfig config = mock(ObservabilityConfig.class); - LogHelper spyLogHelper = spy(new LogHelper(mockSink, TimeProvider.SYSTEM_TIME_PROVIDER)); + LogHelper spyLogHelper = spy(new LogHelper(mockSink)); ConfigFilterHelper mockFilterHelper2 = mock(ConfigFilterHelper.class); InternalLoggingChannelInterceptor.Factory channelInterceptorFactory = new InternalLoggingChannelInterceptor.FactoryImpl(spyLogHelper, mockFilterHelper2); @@ -240,8 +239,8 @@ public void run() { ArgumentCaptor captor = ArgumentCaptor.forClass(GrpcLogRecord.class); verify(mockSink, times(12)).write(captor.capture()); for (GrpcLogRecord record : captor.getAllValues()) { - assertThat(record.getEventType()).isInstanceOf(GrpcLogRecord.EventType.class); - assertThat(record.getEventLogger()).isInstanceOf(GrpcLogRecord.EventLogger.class); + assertThat(record.getType()).isInstanceOf(GrpcLogRecord.EventType.class); + assertThat(record.getLogger()).isInstanceOf(GrpcLogRecord.EventLogger.class); } } catch (IOException e) { throw new AssertionError("Exception while testing logging using mock sink", e);