From bc1896c0b3d91367693dbe6c01c40210e350702b Mon Sep 17 00:00:00 2001 From: jdoherty07 Date: Fri, 10 Nov 2023 17:00:10 +0000 Subject: [PATCH 1/4] adding validation for tracing annotation keys --- powertools-tracing/pom.xml | 11 +++++ .../powertools/tracing/TracingUtils.java | 11 +++++ .../powertools/tracing/TracingUtilsTest.java | 48 +++++++++++++++++++ 3 files changed, 70 insertions(+) diff --git a/powertools-tracing/pom.xml b/powertools-tracing/pom.xml index d98a97e2a..b4dbac719 100644 --- a/powertools-tracing/pom.xml +++ b/powertools-tracing/pom.xml @@ -48,6 +48,10 @@ + + 1.2.11 + + ossrh @@ -115,6 +119,13 @@ assertj-core test + + ch.qos.logback + logback-classic + ${logback.version} + test + + diff --git a/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java b/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java index 9fb021548..618c19f18 100644 --- a/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java +++ b/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java @@ -21,12 +21,15 @@ import com.amazonaws.xray.entities.Subsegment; import com.fasterxml.jackson.databind.ObjectMapper; import java.util.function.Consumer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A class of helper functions to add additional functionality and ease * of use. */ public final class TracingUtils { + private static final Logger LOG = LoggerFactory.getLogger(TracingUtils.class); private static ObjectMapper objectMapper; /** @@ -36,6 +39,7 @@ public final class TracingUtils { * @param value the value of the annotation */ public static void putAnnotation(String key, String value) { + validateAnnotationKey(key); AWSXRay.getCurrentSubsegmentOptional() .ifPresent(segment -> segment.putAnnotation(key, value)); } @@ -47,10 +51,17 @@ public static void putAnnotation(String key, String value) { * @param value the value of the annotation */ public static void putAnnotation(String key, Number value) { + validateAnnotationKey(key); AWSXRay.getCurrentSubsegmentOptional() .ifPresent(segment -> segment.putAnnotation(key, value)); } + private static void validateAnnotationKey(String key) { + if (!key.matches("^[a-zA-Z0-9_]+$")) { + LOG.warn("ignoring annotation with unsupported characters in key: {}", key); + } + } + /** * Put an annotation to the current subsegment with a Boolean value. * diff --git a/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java b/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java index 78283fbc2..a82d85702 100644 --- a/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java +++ b/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java @@ -20,12 +20,18 @@ import static org.mockito.Mockito.verify; import static software.amazon.lambda.powertools.tracing.TracingUtils.withEntitySubsegment; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import com.amazonaws.services.lambda.runtime.Context; import com.amazonaws.xray.AWSXRay; import com.amazonaws.xray.entities.Entity; +import java.util.List; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; class TracingUtilsTest { @@ -123,6 +129,48 @@ void shouldInvokeCodeBlockWrappedWithinSubsegment() { }); } + @Test + void shouldEmitNoLogWarnIfValidCharacterInKey() { + AWSXRay.beginSubsegment("subSegment"); + Logger logger = (Logger) LoggerFactory.getLogger(TracingUtils.class); + + // create and start a ListAppender + ListAppender listAppender = new ListAppender<>(); + listAppender.start(); + + // add the appender to the logger + logger.addAppender(listAppender); + + TracingUtils.putAnnotation("stringKey", "val"); + + List logsList = listAppender.list; + assertThat(AWSXRay.getTraceEntity().getAnnotations()) + .hasSize(1) + .contains( + entry("stringKey", "val") + ); + assertThat(logsList.size()).isZero(); + } + + @Test + void shouldEmitLogWarnIfInvalidCharacterInKey() { + AWSXRay.beginSubsegment("subSegment"); + Logger logger = (Logger) LoggerFactory.getLogger(TracingUtils.class); + + // create and start a ListAppender + ListAppender listAppender = new ListAppender<>(); + listAppender.start(); + + // add the appender to the logger + logger.addAppender(listAppender); + String inputKey = "stringKey with spaces"; + TracingUtils.putAnnotation(inputKey, "val"); + + List logsList = listAppender.list; + assertThat(logsList.get(0).getLevel()).isEqualTo(Level.WARN); + assertThat(logsList.get(0).getMessage()).isEqualTo("ignoring annotation with unsupported characters in key: {}",inputKey); + } + @Test void shouldInvokeCodeBlockWrappedWithinNamespacedSubsegment() { Context test = mock(Context.class); From 217cb1301f1d7eedf3d12bd53365854cac0d06c1 Mon Sep 17 00:00:00 2001 From: jdoherty07 Date: Mon, 13 Nov 2023 09:58:06 +0000 Subject: [PATCH 2/4] updated following PR comments --- .../powertools/tracing/TracingUtils.java | 22 ++++++++++++++----- .../powertools/tracing/TracingUtilsTest.java | 2 +- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java b/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java index 618c19f18..61d347efe 100644 --- a/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java +++ b/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java @@ -39,7 +39,10 @@ public final class TracingUtils { * @param value the value of the annotation */ public static void putAnnotation(String key, String value) { - validateAnnotationKey(key); + if (!isValidateAnnotationKey(key)) { + LOG.warn("Ignoring annotation with unsupported characters in key: {}", key); + return; + } AWSXRay.getCurrentSubsegmentOptional() .ifPresent(segment -> segment.putAnnotation(key, value)); } @@ -51,15 +54,22 @@ public static void putAnnotation(String key, String value) { * @param value the value of the annotation */ public static void putAnnotation(String key, Number value) { - validateAnnotationKey(key); + if (!isValidateAnnotationKey(key)) { + LOG.warn("Ignoring annotation with unsupported characters in key: {}", key); + return; + } AWSXRay.getCurrentSubsegmentOptional() .ifPresent(segment -> segment.putAnnotation(key, value)); } - private static void validateAnnotationKey(String key) { - if (!key.matches("^[a-zA-Z0-9_]+$")) { - LOG.warn("ignoring annotation with unsupported characters in key: {}", key); - } + /** + Make sure that the annotation key is valid according to + the documentation. + + Annotation keys that are added that are invalid are ignored by x-ray. + **/ + private static boolean isValidateAnnotationKey(String key) { + return key.matches("^[a-zA-Z0-9_]+$"); } /** diff --git a/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java b/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java index a82d85702..1bb639e97 100644 --- a/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java +++ b/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java @@ -168,7 +168,7 @@ void shouldEmitLogWarnIfInvalidCharacterInKey() { List logsList = listAppender.list; assertThat(logsList.get(0).getLevel()).isEqualTo(Level.WARN); - assertThat(logsList.get(0).getMessage()).isEqualTo("ignoring annotation with unsupported characters in key: {}",inputKey); + assertThat(logsList.get(0).getMessage()).isEqualTo("Ignoring annotation with unsupported characters in key: {}",inputKey); } @Test From 554856a44145f810d87fb76998ebf890d30a4e2d Mon Sep 17 00:00:00 2001 From: jdoherty07 Date: Mon, 13 Nov 2023 10:02:40 +0000 Subject: [PATCH 3/4] correcting spelling mistake --- .../amazon/lambda/powertools/tracing/TracingUtils.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java b/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java index 61d347efe..47c4e7422 100644 --- a/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java +++ b/powertools-tracing/src/main/java/software/amazon/lambda/powertools/tracing/TracingUtils.java @@ -39,7 +39,7 @@ public final class TracingUtils { * @param value the value of the annotation */ public static void putAnnotation(String key, String value) { - if (!isValidateAnnotationKey(key)) { + if (!isValidAnnotationKey(key)) { LOG.warn("Ignoring annotation with unsupported characters in key: {}", key); return; } @@ -54,7 +54,7 @@ public static void putAnnotation(String key, String value) { * @param value the value of the annotation */ public static void putAnnotation(String key, Number value) { - if (!isValidateAnnotationKey(key)) { + if (!isValidAnnotationKey(key)) { LOG.warn("Ignoring annotation with unsupported characters in key: {}", key); return; } @@ -68,7 +68,7 @@ public static void putAnnotation(String key, Number value) { Annotation keys that are added that are invalid are ignored by x-ray. **/ - private static boolean isValidateAnnotationKey(String key) { + private static boolean isValidAnnotationKey(String key) { return key.matches("^[a-zA-Z0-9_]+$"); } From 48b00fb601ca8f4abef4281c3e8fafd835a7b0c9 Mon Sep 17 00:00:00 2001 From: jdoherty07 Date: Tue, 14 Nov 2023 13:28:18 +0000 Subject: [PATCH 4/4] removed logback testing solution and test the annotation not logging --- powertools-tracing/pom.xml | 11 ---- .../powertools/tracing/TracingUtilsTest.java | 51 ++++--------------- 2 files changed, 10 insertions(+), 52 deletions(-) diff --git a/powertools-tracing/pom.xml b/powertools-tracing/pom.xml index b4dbac719..d98a97e2a 100644 --- a/powertools-tracing/pom.xml +++ b/powertools-tracing/pom.xml @@ -48,10 +48,6 @@ - - 1.2.11 - - ossrh @@ -119,13 +115,6 @@ assertj-core test - - ch.qos.logback - logback-classic - ${logback.version} - test - - diff --git a/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java b/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java index 1bb639e97..01f25f37a 100644 --- a/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java +++ b/powertools-tracing/src/test/java/software/amazon/lambda/powertools/tracing/TracingUtilsTest.java @@ -20,21 +20,14 @@ import static org.mockito.Mockito.verify; import static software.amazon.lambda.powertools.tracing.TracingUtils.withEntitySubsegment; -import ch.qos.logback.classic.Level; -import ch.qos.logback.classic.Logger; -import ch.qos.logback.classic.spi.ILoggingEvent; -import ch.qos.logback.core.read.ListAppender; import com.amazonaws.services.lambda.runtime.Context; import com.amazonaws.xray.AWSXRay; import com.amazonaws.xray.entities.Entity; -import java.util.List; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.slf4j.LoggerFactory; class TracingUtilsTest { - @BeforeEach void setUp() { AWSXRay.beginSegment("test"); @@ -130,45 +123,21 @@ void shouldInvokeCodeBlockWrappedWithinSubsegment() { } @Test - void shouldEmitNoLogWarnIfValidCharacterInKey() { + void shouldNotAddAnnotationIfInvalidCharacterInKey() { AWSXRay.beginSubsegment("subSegment"); - Logger logger = (Logger) LoggerFactory.getLogger(TracingUtils.class); - - // create and start a ListAppender - ListAppender listAppender = new ListAppender<>(); - listAppender.start(); - - // add the appender to the logger - logger.addAppender(listAppender); - - TracingUtils.putAnnotation("stringKey", "val"); - - List logsList = listAppender.list; - assertThat(AWSXRay.getTraceEntity().getAnnotations()) - .hasSize(1) - .contains( - entry("stringKey", "val") - ); - assertThat(logsList.size()).isZero(); + String inputKey = "stringKey with spaces"; + TracingUtils.putAnnotation(inputKey, "val"); + AWSXRay.getCurrentSubsegmentOptional() + .ifPresent(segment -> assertThat(segment.getAnnotations()).size().isEqualTo(0)); } @Test - void shouldEmitLogWarnIfInvalidCharacterInKey() { + void shouldAddAnnotationIfValidCharactersInKey() { AWSXRay.beginSubsegment("subSegment"); - Logger logger = (Logger) LoggerFactory.getLogger(TracingUtils.class); - - // create and start a ListAppender - ListAppender listAppender = new ListAppender<>(); - listAppender.start(); - - // add the appender to the logger - logger.addAppender(listAppender); - String inputKey = "stringKey with spaces"; + String inputKey = "validKey"; TracingUtils.putAnnotation(inputKey, "val"); - - List logsList = listAppender.list; - assertThat(logsList.get(0).getLevel()).isEqualTo(Level.WARN); - assertThat(logsList.get(0).getMessage()).isEqualTo("Ignoring annotation with unsupported characters in key: {}",inputKey); + AWSXRay.getCurrentSubsegmentOptional() + .ifPresent(segment -> assertThat(segment.getAnnotations()).size().isEqualTo(1)); } @Test @@ -269,4 +238,4 @@ void shouldInvokeCodeBlockWrappedWithinNamespacedEntitySubsegment() throws Inter .containsEntry("key", "val"); }); } -} \ No newline at end of file +}