feat(system-level-exclusion-rules): system level exclude span rules (#…

…133) * feat(system-level-exclusion-rules): system level exclude span rules * add new test * refactor * address review comments * add comment * address review comments * nit and lists
hypertrace · Sep 2, 2022 · f786710 · f786710
1 parent 10eec87
commit f786710
Show file tree

Hide file tree

Showing 8 changed files with 335 additions and 35 deletions.
diff --git a/config-service-factory/src/main/java/org/hypertrace/config/service/ConfigServiceFactory.java b/config-service-factory/src/main/java/org/hypertrace/config/service/ConfigServiceFactory.java
@@ -66,7 +66,7 @@ public List<GrpcPlatformService> buildServices(
             new EventConditionConfigServiceImpl(localChannel, configChangeEventGenerator),
             new NotificationRuleConfigServiceImpl(localChannel, configChangeEventGenerator),
             new NotificationChannelConfigServiceImpl(localChannel, configChangeEventGenerator),
-            SpanProcessingConfigServiceFactory.build(localChannel))
+            SpanProcessingConfigServiceFactory.build(localChannel, config))
         .map(GrpcPlatformService::new)
         .collect(Collectors.toUnmodifiableList());
   }

diff --git a/...oto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto b/...oto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto
@@ -87,6 +87,7 @@ message ExcludeSpanRuleInfo {
   string name = 1;
   SpanFilter filter = 2;
   bool disabled = 3;
+  RuleType type = 4;
 }
 
 message ExcludeSpanRuleMetadata {
@@ -101,6 +102,12 @@ message UpdateExcludeSpanRule {
   bool disabled = 4;
 }
 
+enum RuleType {
+  RULE_TYPE_UNSPECIFIED = 0;
+  RULE_TYPE_SYSTEM = 1;
+  RULE_TYPE_USER = 2;
+}
+
 message CreateApiNamingRuleRequest {
   ApiNamingRuleInfo rule_info = 1;
 }

diff --git a/span-processing-config-service-impl/build.gradle.kts b/span-processing-config-service-impl/build.gradle.kts
@@ -18,6 +18,7 @@ dependencies {
   implementation(libs.guava)
   implementation(libs.slf4j.api)
   implementation(libs.google.re2j)
+  implementation(libs.typesafe.config)
 
   annotationProcessor(libs.lombok)
   compileOnly(libs.lombok)

diff --git a/...ava/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceFactory.java b/...ava/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceFactory.java
@@ -2,12 +2,14 @@
 
 import com.google.inject.Guice;
 import com.google.inject.Injector;
+import com.typesafe.config.Config;
 import io.grpc.BindableService;
 import io.grpc.Channel;
 
 public class SpanProcessingConfigServiceFactory {
-  public static BindableService build(Channel channel) {
-    Injector injector = Guice.createInjector(new SpanProcessingConfigServiceModule(channel));
+  public static BindableService build(Channel channel, Config config) {
+    Injector injector =
+        Guice.createInjector(new SpanProcessingConfigServiceModule(channel, config));
     return injector.getInstance(BindableService.class);
   }
 }
diff --git a/...n/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java b/...n/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java
@@ -1,9 +1,23 @@
 package org.hypertrace.span.processing.config.service;
 
+import static org.hypertrace.span.processing.config.service.v1.RuleType.RULE_TYPE_SYSTEM;
+
 import com.google.inject.Inject;
+import com.google.protobuf.util.JsonFormat;
+import com.typesafe.config.Config;
 import io.grpc.Status;
 import io.grpc.stub.StreamObserver;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
 import java.util.UUID;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import lombok.SneakyThrows;
 import lombok.extern.slf4j.Slf4j;
 import org.hypertrace.config.objectstore.ContextualConfigObject;
 import org.hypertrace.core.grpcutils.context.RequestContext;
@@ -43,21 +57,42 @@
 @Slf4j
 class SpanProcessingConfigServiceImpl
     extends SpanProcessingConfigServiceGrpc.SpanProcessingConfigServiceImplBase {
+
+  private static final String SYSTEM_EXCLUDE_SPAN_RULES =
+      "span.processing.config.service.system.exclude.span.rules";
+
   private final SpanProcessingConfigRequestValidator validator;
   private final ExcludeSpanRulesConfigStore excludeSpanRulesConfigStore;
   private final TimestampConverter timestampConverter;
   private final ApiNamingRulesManager apiNamingRulesManager;
+  private List<ExcludeSpanRule> systemExcludeSpanRules;
+  private Map<String, ExcludeSpanRule> systemExcludeSpanRuleIdToRuleMap;
 
   @Inject
   SpanProcessingConfigServiceImpl(
       ExcludeSpanRulesConfigStore excludeSpanRulesConfigStore,
       SpanProcessingConfigRequestValidator requestValidator,
       TimestampConverter timestampConverter,
-      ApiNamingRulesManager apiNamingRulesManager) {
+      ApiNamingRulesManager apiNamingRulesManager,
+      Config config) {
     this.validator = requestValidator;
     this.excludeSpanRulesConfigStore = excludeSpanRulesConfigStore;
     this.timestampConverter = timestampConverter;
     this.apiNamingRulesManager = apiNamingRulesManager;
+
+    buildSystemExcludeSpanRuleConfigs(config);
+  }
+
+  private void buildSystemExcludeSpanRuleConfigs(Config config) {
+    if (!config.hasPath(SYSTEM_EXCLUDE_SPAN_RULES)) {
+      systemExcludeSpanRules = Collections.emptyList();
+      systemExcludeSpanRuleIdToRuleMap = Collections.emptyMap();
+      return;
+    }
+    List<? extends com.typesafe.config.ConfigObject> systemExcludeSpanRuleObjects =
+        config.getObjectList(SYSTEM_EXCLUDE_SPAN_RULES);
+    systemExcludeSpanRules = buildSystemExcludeSpanRules(systemExcludeSpanRuleObjects);
+    systemExcludeSpanRuleIdToRuleMap = buildExcludeSpanRuleIdToRuleMap(systemExcludeSpanRules);
   }
 
   @Override
@@ -68,10 +103,15 @@ public void getAllExcludeSpanRules(
       RequestContext requestContext = RequestContext.CURRENT.get();
       this.validator.validateOrThrow(requestContext, request);
 
+      List<ExcludeSpanRuleDetails> userExcludeSpanRules =
+          excludeSpanRulesConfigStore.getAllData(requestContext);
+
+      // reorder user exclude span rules and system exclude span rules as per expected priority
+      List<ExcludeSpanRuleDetails> excludeSpanRules =
+          reorderExcludeSpanRules(userExcludeSpanRules, systemExcludeSpanRuleIdToRuleMap);
+
       responseObserver.onNext(
-          GetAllExcludeSpanRulesResponse.newBuilder()
-              .addAllRuleDetails(excludeSpanRulesConfigStore.getAllData(requestContext))
-              .build());
+          GetAllExcludeSpanRulesResponse.newBuilder().addAllRuleDetails(excludeSpanRules).build());
       responseObserver.onCompleted();
     } catch (Exception e) {
       log.error("Unable to get all exclude span rules for request: {}", request, e);
@@ -116,10 +156,18 @@ public void updateExcludeSpanRule(
       this.validator.validateOrThrow(requestContext, request);
 
       UpdateExcludeSpanRule updateExcludeSpanRule = request.getRule();
+
+      // check if the rule already exists. If not, check if it is a system config. If yes, use
+      // that. Else, error out.
       ExcludeSpanRule existingRule =
           this.excludeSpanRulesConfigStore
               .getData(requestContext, updateExcludeSpanRule.getId())
+              .or(() -> getSystemExcludeSpanRule(updateExcludeSpanRule.getId()))
               .orElseThrow(Status.NOT_FOUND::asException);
+
+      // if the rule being updated is a system exclude rule, only disabled field update should be
+      // allowed
+      validateUpdateExcludeSpanRule(existingRule, updateExcludeSpanRule);
       ExcludeSpanRule updatedRule = buildUpdatedRule(existingRule, updateExcludeSpanRule);
 
       responseObserver.onNext(
@@ -143,6 +191,11 @@ public void deleteExcludeSpanRule(
       RequestContext requestContext = RequestContext.CURRENT.get();
       this.validator.validateOrThrow(requestContext, request);
 
+      // do not allow deleting system exclude rules
+      if (systemExcludeSpanRuleIdToRuleMap.containsKey(request.getId())) {
+        throw Status.INVALID_ARGUMENT.asRuntimeException();
+      }
+
       // TODO: need to handle priorities
       this.excludeSpanRulesConfigStore
           .deleteObject(requestContext, request.getId())
@@ -156,6 +209,83 @@ public void deleteExcludeSpanRule(
     }
   }
 
+  @SneakyThrows
+  private ExcludeSpanRule buildExcludeSpanRuleFromConfig(
+      com.typesafe.config.ConfigObject configObject) {
+    String jsonString = configObject.render();
+    ExcludeSpanRule.Builder builder = ExcludeSpanRule.newBuilder();
+    JsonFormat.parser().merge(jsonString, builder);
+    return builder.build();
+  }
+
+  private Optional<ExcludeSpanRule> getSystemExcludeSpanRule(String id) {
+    if (systemExcludeSpanRuleIdToRuleMap.containsKey(id)) {
+      return Optional.of(systemExcludeSpanRuleIdToRuleMap.get(id));
+    }
+    return Optional.empty();
+  }
+
+  private List<ExcludeSpanRuleDetails> reorderExcludeSpanRules(
+      List<ExcludeSpanRuleDetails> userExcludeSpanRules,
+      Map<String, ExcludeSpanRule> systemExcludeSpanRuleIdToRuleMap) {
+    // This method sets the priority of rules. User exclude span rules -> user overridden system
+    // exclude span rules -> non overridden system exclude span rules
+    List<ExcludeSpanRuleDetails> excludeSpanRules = new ArrayList<>();
+    List<ExcludeSpanRuleDetails> overriddenSystemExcludeSpanRules = new ArrayList<>();
+    Set<String> overriddenSystemExcludeSpanRuleIds = new HashSet<>();
+
+    for (ExcludeSpanRuleDetails excludeSpanRule : userExcludeSpanRules) {
+      String id = excludeSpanRule.getRule().getId();
+      if (systemExcludeSpanRuleIdToRuleMap.containsKey(id)) {
+        // user overridden system exclude span rules
+        overriddenSystemExcludeSpanRules.add(excludeSpanRule);
+        overriddenSystemExcludeSpanRuleIds.add(id);
+      } else {
+        // user exclude span rules
+        excludeSpanRules.add(excludeSpanRule);
+      }
+    }
+
+    // non overridden system exclude span rules
+    List<ExcludeSpanRuleDetails> nonOverriddenSystemExcludeSpanRules =
+        systemExcludeSpanRules.stream()
+            .filter(
+                excludeSpanRule ->
+                    !overriddenSystemExcludeSpanRuleIds.contains(excludeSpanRule.getId()))
+            .map(
+                excludeSpanRule ->
+                    ExcludeSpanRuleDetails.newBuilder().setRule(excludeSpanRule).build())
+            .collect(Collectors.toUnmodifiableList());
+
+    excludeSpanRules.addAll(overriddenSystemExcludeSpanRules);
+    excludeSpanRules.addAll(nonOverriddenSystemExcludeSpanRules);
+    return excludeSpanRules;
+  }
+
+  private List<ExcludeSpanRule> buildSystemExcludeSpanRules(
+      List<? extends com.typesafe.config.ConfigObject> configObjects) {
+    return configObjects.stream()
+        .map(this::buildExcludeSpanRuleFromConfig)
+        .collect(Collectors.toUnmodifiableList());
+  }
+
+  private Map<String, ExcludeSpanRule> buildExcludeSpanRuleIdToRuleMap(
+      List<ExcludeSpanRule> excludeSpanRules) {
+    return excludeSpanRules.stream()
+        .collect(Collectors.toUnmodifiableMap(ExcludeSpanRule::getId, Function.identity()));
+  }
+
+  private void validateUpdateExcludeSpanRule(
+      ExcludeSpanRule existingRule, UpdateExcludeSpanRule updateExcludeSpanRule) {
+    ExcludeSpanRuleInfo ruleInfo = existingRule.getRuleInfo();
+    if (RULE_TYPE_SYSTEM.equals(ruleInfo.getType())) {
+      if (!updateExcludeSpanRule.getName().equals(ruleInfo.getName())
+          || !updateExcludeSpanRule.getFilter().equals(ruleInfo.getFilter())) {
+        throw Status.INVALID_ARGUMENT.asRuntimeException();
+      }
+    }
+  }
+
   private ExcludeSpanRule buildUpdatedRule(
       ExcludeSpanRule existingRule, UpdateExcludeSpanRule updateExcludeSpanRule) {
     return ExcludeSpanRule.newBuilder(existingRule)
@@ -164,6 +294,7 @@ private ExcludeSpanRule buildUpdatedRule(
                 .setName(updateExcludeSpanRule.getName())
                 .setFilter(updateExcludeSpanRule.getFilter())
                 .setDisabled(updateExcludeSpanRule.getDisabled())
+                .setType(existingRule.getRuleInfo().getType())
                 .build())
         .build();
   }

diff --git a/...java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceModule.java b/...java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceModule.java
@@ -2,6 +2,7 @@
 
 import com.google.inject.AbstractModule;
 import com.google.inject.Provides;
+import com.typesafe.config.Config;
 import io.grpc.BindableService;
 import io.grpc.Channel;
 import org.hypertrace.config.service.v1.ConfigServiceGrpc;
@@ -10,14 +11,17 @@
 
 public class SpanProcessingConfigServiceModule extends AbstractModule {
   private final Channel channel;
+  private final Config config;
 
-  SpanProcessingConfigServiceModule(Channel channel) {
+  SpanProcessingConfigServiceModule(Channel channel, Config config) {
     this.channel = channel;
+    this.config = config;
   }
 
   @Override
   protected void configure() {
     bind(BindableService.class).to(SpanProcessingConfigServiceImpl.class);
+    bind(Config.class).toInstance(config);
 
     install(new ApiNamingRulesManagerModule());
   }