From 201695a7f2fe4f4479ac2976c58eafad1512ac0b Mon Sep 17 00:00:00 2001 From: SrikarMannepalli Date: Fri, 25 Feb 2022 12:53:18 +0530 Subject: [PATCH 1/4] add enable/disable support --- .../v1/span_processing_config_service.proto | 4 +++- .../SpanProcessingConfigServiceImpl.java | 19 ++++++++++++------- .../SpanProcessingConfigRequestValidator.java | 5 ++--- .../SpanProcessingConfigServiceImplTest.java | 14 +++++--------- .../SpanProcessingRequestValidatorTest.java | 16 ++++------------ 5 files changed, 26 insertions(+), 32 deletions(-) diff --git a/span-processing-config-service-api/src/main/proto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto b/span-processing-config-service-api/src/main/proto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto index b27060dd..8d88ff52 100644 --- a/span-processing-config-service-api/src/main/proto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto +++ b/span-processing-config-service-api/src/main/proto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto @@ -57,6 +57,7 @@ message ExcludeSpanRuleDetails { message ExcludeSpanRuleInfo { string name = 1; SpanFilter filter = 2; + bool disabled = 3; } message ExcludeSpanRuleMetadata { @@ -66,8 +67,9 @@ message ExcludeSpanRuleMetadata { message UpdateExcludeSpanRule { string id = 1; - string name = 2; + optional string name = 2; SpanFilter filter = 3; + optional bool disabled = 4; } message SpanFilter { diff --git a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java index f8e99e7d..a8692ab8 100644 --- a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java +++ b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java @@ -139,13 +139,18 @@ public void deleteExcludeSpanRule( private ExcludeSpanRule buildUpdatedRule( ExcludeSpanRule existingRule, UpdateExcludeSpanRule updateExcludeSpanRule) { - return ExcludeSpanRule.newBuilder(existingRule) - .setRuleInfo( - ExcludeSpanRuleInfo.newBuilder() - .setName(updateExcludeSpanRule.getName()) - .setFilter(updateExcludeSpanRule.getFilter()) - .build()) - .build(); + ExcludeSpanRuleInfo.Builder excludeSpanRuleInfoBuilder = + ExcludeSpanRuleInfo.newBuilder(existingRule.getRuleInfo()); + if (updateExcludeSpanRule.hasName()) { + excludeSpanRuleInfoBuilder.setName(updateExcludeSpanRule.getName()); + } + if (updateExcludeSpanRule.hasFilter()) { + excludeSpanRuleInfoBuilder.setFilter(updateExcludeSpanRule.getFilter()); + } + if (updateExcludeSpanRule.hasDisabled()) { + excludeSpanRuleInfoBuilder.setDisabled(updateExcludeSpanRule.getDisabled()); + } + return ExcludeSpanRule.newBuilder(existingRule).setRuleInfo(excludeSpanRuleInfoBuilder).build(); } private ExcludeSpanRuleDetails buildExcludeSpanRuleDetails( diff --git a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java index a97bb39b..b2763861 100644 --- a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java +++ b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java @@ -38,14 +38,13 @@ public void validateOrThrow(RequestContext requestContext, DeleteExcludeSpanRule private void validateData(ExcludeSpanRuleInfo excludeSpanRuleInfo) { validateNonDefaultPresenceOrThrow(excludeSpanRuleInfo, ExcludeSpanRuleInfo.NAME_FIELD_NUMBER); + validateNonDefaultPresenceOrThrow( + excludeSpanRuleInfo, ExcludeSpanRuleInfo.DISABLED_FIELD_NUMBER); this.validateSpanFilter(excludeSpanRuleInfo.getFilter()); } private void validateUpdateRule(UpdateExcludeSpanRule updateExcludeSpanRule) { validateNonDefaultPresenceOrThrow(updateExcludeSpanRule, UpdateExcludeSpanRule.ID_FIELD_NUMBER); - validateNonDefaultPresenceOrThrow( - updateExcludeSpanRule, UpdateExcludeSpanRule.NAME_FIELD_NUMBER); - this.validateSpanFilter(updateExcludeSpanRule.getFilter()); } private void validateSpanFilter(SpanFilter filter) { diff --git a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImplTest.java b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImplTest.java index a97617db..bcb2a487 100644 --- a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImplTest.java +++ b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImplTest.java @@ -1,6 +1,7 @@ package org.hypertrace.span.processing.config.service; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; @@ -81,6 +82,7 @@ void testCrud() { .setRuleInfo( ExcludeSpanRuleInfo.newBuilder() .setName("ruleName1") + .setDisabled(true) .setFilter( SpanFilter.newBuilder() .setRelationalSpanFilter( @@ -107,6 +109,7 @@ void testCrud() { .setRuleInfo( ExcludeSpanRuleInfo.newBuilder() .setName("ruleName2") + .setDisabled(true) .setFilter( SpanFilter.newBuilder() .setRelationalSpanFilter( @@ -139,19 +142,12 @@ void testCrud() { UpdateExcludeSpanRule.newBuilder() .setId(firstCreatedExcludeSpanRule.getId()) .setName("updatedRuleName1") - .setFilter( - SpanFilter.newBuilder() - .setRelationalSpanFilter( - RelationalSpanFilterExpression.newBuilder() - .setField(Field.FIELD_SERVICE_NAME) - .setOperator( - RelationalOperator.RELATIONAL_OPERATOR_CONTAINS) - .setRightOperand( - SpanFilterValue.newBuilder().setStringValue("a"))))) + .setDisabled(false)) .build()) .getRuleDetails() .getRule(); assertEquals("updatedRuleName1", updatedFirstExcludeSpanRule.getRuleInfo().getName()); + assertFalse(updatedFirstExcludeSpanRule.getRuleInfo().getDisabled()); excludeSpanRules = this.spanProcessingConfigServiceStub diff --git a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java index 1343b4dd..8489714e 100644 --- a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java +++ b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java @@ -83,7 +83,7 @@ void validatesCreateRequest() { when(mockRequestContext.getTenantId()).thenReturn(Optional.of(TEST_TENANT_ID)); assertInvalidArgStatusContaining( - "ExcludeSpanRuleInfo", + "ExcludeSpanRuleInfo.name", () -> validator.validateOrThrow( mockRequestContext, @@ -92,12 +92,12 @@ void validatesCreateRequest() { .build())); assertInvalidArgStatusContaining( - "ExcludeSpanRuleInfo.name", + "ExcludeSpanRuleInfo.disabled", () -> validator.validateOrThrow( mockRequestContext, CreateExcludeSpanRuleRequest.newBuilder() - .setRuleInfo(ExcludeSpanRuleInfo.newBuilder().build()) + .setRuleInfo(ExcludeSpanRuleInfo.newBuilder().setName("name").build()) .build())); assertDoesNotThrow( @@ -108,6 +108,7 @@ void validatesCreateRequest() { .setRuleInfo( ExcludeSpanRuleInfo.newBuilder() .setName("name") + .setDisabled(true) .setFilter( SpanFilter.newBuilder() .setRelationalSpanFilter( @@ -143,15 +144,6 @@ void validatesUpdateRequest() { .setRule(UpdateExcludeSpanRule.newBuilder().setName("name").build()) .build())); - assertInvalidArgStatusContaining( - "UpdateExcludeSpanRule.name", - () -> - validator.validateOrThrow( - mockRequestContext, - UpdateExcludeSpanRuleRequest.newBuilder() - .setRule(UpdateExcludeSpanRule.newBuilder().setId("id").build()) - .build())); - assertDoesNotThrow( () -> validator.validateOrThrow( From 9339569a101078edfa25572cdd00e7bb1b0ad7d3 Mon Sep 17 00:00:00 2001 From: SrikarMannepalli Date: Fri, 25 Feb 2022 14:39:59 +0530 Subject: [PATCH 2/4] address review comments --- .../v1/span_processing_config_service.proto | 4 ++-- .../SpanProcessingConfigServiceImpl.java | 20 ++++++++----------- .../SpanProcessingConfigRequestValidator.java | 3 +++ .../SpanProcessingConfigServiceImplTest.java | 11 +++++++++- 4 files changed, 23 insertions(+), 15 deletions(-) diff --git a/span-processing-config-service-api/src/main/proto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto b/span-processing-config-service-api/src/main/proto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto index 8d88ff52..e11132e1 100644 --- a/span-processing-config-service-api/src/main/proto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto +++ b/span-processing-config-service-api/src/main/proto/org/hypertrace/span/processing/config/service/v1/span_processing_config_service.proto @@ -67,9 +67,9 @@ message ExcludeSpanRuleMetadata { message UpdateExcludeSpanRule { string id = 1; - optional string name = 2; + string name = 2; SpanFilter filter = 3; - optional bool disabled = 4; + bool disabled = 4; } message SpanFilter { diff --git a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java index a8692ab8..e310c414 100644 --- a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java +++ b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImpl.java @@ -139,18 +139,14 @@ public void deleteExcludeSpanRule( private ExcludeSpanRule buildUpdatedRule( ExcludeSpanRule existingRule, UpdateExcludeSpanRule updateExcludeSpanRule) { - ExcludeSpanRuleInfo.Builder excludeSpanRuleInfoBuilder = - ExcludeSpanRuleInfo.newBuilder(existingRule.getRuleInfo()); - if (updateExcludeSpanRule.hasName()) { - excludeSpanRuleInfoBuilder.setName(updateExcludeSpanRule.getName()); - } - if (updateExcludeSpanRule.hasFilter()) { - excludeSpanRuleInfoBuilder.setFilter(updateExcludeSpanRule.getFilter()); - } - if (updateExcludeSpanRule.hasDisabled()) { - excludeSpanRuleInfoBuilder.setDisabled(updateExcludeSpanRule.getDisabled()); - } - return ExcludeSpanRule.newBuilder(existingRule).setRuleInfo(excludeSpanRuleInfoBuilder).build(); + return ExcludeSpanRule.newBuilder(existingRule) + .setRuleInfo( + ExcludeSpanRuleInfo.newBuilder() + .setName(updateExcludeSpanRule.getName()) + .setFilter(updateExcludeSpanRule.getFilter()) + .setDisabled(updateExcludeSpanRule.getDisabled()) + .build()) + .build(); } private ExcludeSpanRuleDetails buildExcludeSpanRuleDetails( diff --git a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java index b2763861..2a055bd4 100644 --- a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java +++ b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java @@ -45,6 +45,9 @@ private void validateData(ExcludeSpanRuleInfo excludeSpanRuleInfo) { private void validateUpdateRule(UpdateExcludeSpanRule updateExcludeSpanRule) { validateNonDefaultPresenceOrThrow(updateExcludeSpanRule, UpdateExcludeSpanRule.ID_FIELD_NUMBER); + validateNonDefaultPresenceOrThrow( + updateExcludeSpanRule, UpdateExcludeSpanRule.NAME_FIELD_NUMBER); + this.validateSpanFilter(updateExcludeSpanRule.getFilter()); } private void validateSpanFilter(SpanFilter filter) { diff --git a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImplTest.java b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImplTest.java index bcb2a487..aa18bf9f 100644 --- a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImplTest.java +++ b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/SpanProcessingConfigServiceImplTest.java @@ -142,7 +142,16 @@ void testCrud() { UpdateExcludeSpanRule.newBuilder() .setId(firstCreatedExcludeSpanRule.getId()) .setName("updatedRuleName1") - .setDisabled(false)) + .setDisabled(false) + .setFilter( + SpanFilter.newBuilder() + .setRelationalSpanFilter( + RelationalSpanFilterExpression.newBuilder() + .setField(Field.FIELD_SERVICE_NAME) + .setOperator( + RelationalOperator.RELATIONAL_OPERATOR_CONTAINS) + .setRightOperand( + SpanFilterValue.newBuilder().setStringValue("a"))))) .build()) .getRuleDetails() .getRule(); From 7226cba9f461ccef490a7c402706e82245914c9e Mon Sep 17 00:00:00 2001 From: SrikarMannepalli Date: Fri, 25 Feb 2022 14:41:08 +0530 Subject: [PATCH 3/4] fix bug --- .../validation/SpanProcessingConfigRequestValidator.java | 2 -- .../validation/SpanProcessingRequestValidatorTest.java | 9 --------- 2 files changed, 11 deletions(-) diff --git a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java index 2a055bd4..a97bb39b 100644 --- a/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java +++ b/span-processing-config-service-impl/src/main/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingConfigRequestValidator.java @@ -38,8 +38,6 @@ public void validateOrThrow(RequestContext requestContext, DeleteExcludeSpanRule private void validateData(ExcludeSpanRuleInfo excludeSpanRuleInfo) { validateNonDefaultPresenceOrThrow(excludeSpanRuleInfo, ExcludeSpanRuleInfo.NAME_FIELD_NUMBER); - validateNonDefaultPresenceOrThrow( - excludeSpanRuleInfo, ExcludeSpanRuleInfo.DISABLED_FIELD_NUMBER); this.validateSpanFilter(excludeSpanRuleInfo.getFilter()); } diff --git a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java index 8489714e..0ea27744 100644 --- a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java +++ b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java @@ -91,15 +91,6 @@ void validatesCreateRequest() { .setRuleInfo(ExcludeSpanRuleInfo.newBuilder().build()) .build())); - assertInvalidArgStatusContaining( - "ExcludeSpanRuleInfo.disabled", - () -> - validator.validateOrThrow( - mockRequestContext, - CreateExcludeSpanRuleRequest.newBuilder() - .setRuleInfo(ExcludeSpanRuleInfo.newBuilder().setName("name").build()) - .build())); - assertDoesNotThrow( () -> validator.validateOrThrow( From e92eb93edc94c641e2ddb1b671dd3cf207bec81b Mon Sep 17 00:00:00 2001 From: SrikarMannepalli Date: Fri, 25 Feb 2022 14:42:11 +0530 Subject: [PATCH 4/4] add test --- .../validation/SpanProcessingRequestValidatorTest.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java index 0ea27744..03f572f5 100644 --- a/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java +++ b/span-processing-config-service-impl/src/test/java/org/hypertrace/span/processing/config/service/validation/SpanProcessingRequestValidatorTest.java @@ -135,6 +135,15 @@ void validatesUpdateRequest() { .setRule(UpdateExcludeSpanRule.newBuilder().setName("name").build()) .build())); + assertInvalidArgStatusContaining( + "UpdateExcludeSpanRule.name", + () -> + validator.validateOrThrow( + mockRequestContext, + UpdateExcludeSpanRuleRequest.newBuilder() + .setRule(UpdateExcludeSpanRule.newBuilder().setId("id").build()) + .build())); + assertDoesNotThrow( () -> validator.validateOrThrow(