From cb7f6f96d9e65c9d0478c09ad9f5a86765b60212 Mon Sep 17 00:00:00 2001 From: Qi Chen <19492223+chenqi0805@users.noreply.github.com> Date: Thu, 17 Mar 2022 15:48:03 -0500 Subject: [PATCH 1/2] MAINT: remove unused fromSpan Signed-off-by: Qi Chen <19492223+chenqi0805@users.noreply.github.com> --- .../dataprepper/model/event/JacksonEvent.java | 6 +- .../model/trace/DefaultTraceGroupFields.java | 8 - .../dataprepper/model/trace/JacksonSpan.java | 68 ++++---- .../amazon/dataprepper/model/trace/Span.java | 14 ++ .../trace/DefaultTraceGroupFieldsTest.java | 48 ------ .../model/trace/JacksonSpanTest.java | 149 +++++++++--------- 6 files changed, 135 insertions(+), 158 deletions(-) diff --git a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java index f6c1952fb0..92727962eb 100644 --- a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java +++ b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java @@ -10,8 +10,8 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.ObjectReader; import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.ObjectReader; import com.fasterxml.jackson.databind.node.ObjectNode; import com.fasterxml.jackson.databind.type.TypeFactory; import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; @@ -104,6 +104,10 @@ private JsonNode getInitialJsonNode(final Object data) { return mapper.valueToTree(data); } + protected JsonNode getJsonNode() { + return jsonNode; + } + /** * Adds or updates the key with a given value in the Event. * @param key where the value will be set diff --git a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/DefaultTraceGroupFields.java b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/DefaultTraceGroupFields.java index 47cadba273..64c16a352e 100644 --- a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/DefaultTraceGroupFields.java +++ b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/DefaultTraceGroupFields.java @@ -5,9 +5,6 @@ package com.amazon.dataprepper.model.trace; -import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.base.Preconditions.checkNotNull; - /** * The default implementation of {@link TraceGroupFields}, the attributes associated with an entire trace. * @@ -24,11 +21,6 @@ public class DefaultTraceGroupFields implements TraceGroupFields { private DefaultTraceGroupFields(final Builder builder) { - checkNotNull(builder.durationInNanos, "durationInNanos cannot be null"); - checkNotNull(builder.statusCode, "statusCode cannot be null"); - checkNotNull(builder.endTime, "endTime cannot be null"); - checkArgument(!builder.endTime.isEmpty(), "endTime cannot be an empty string"); - this.endTime = builder.endTime; this.durationInNanos = builder.durationInNanos; this.statusCode = builder.statusCode; diff --git a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/JacksonSpan.java b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/JacksonSpan.java index 1229406e00..b36d47e97e 100644 --- a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/JacksonSpan.java +++ b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/JacksonSpan.java @@ -7,15 +7,19 @@ import com.amazon.dataprepper.model.event.EventType; import com.amazon.dataprepper.model.event.JacksonEvent; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ObjectNode; import java.util.Arrays; import java.util.HashMap; +import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; /** * A Jackson implementation for {@link Span}. This class extends the {@link JacksonEvent}. @@ -43,10 +47,10 @@ public class JacksonSpan extends JacksonEvent implements Span { private static final String DURATION_IN_NANOS_KEY = "durationInNanos"; private static final String TRACE_GROUP_FIELDS_KEY = "traceGroupFields"; + private static final List REQUIRED_KEYS = Arrays.asList(TRACE_GROUP_KEY); private static final List - REQUIRED_NON_EMPTY_KEYS = Arrays.asList(TRACE_ID_KEY, SPAN_ID_KEY, TRACE_STATE_KEY, PARENT_SPAN_ID_KEY, PARENT_SPAN_ID_KEY, - NAME_KEY, KIND_KEY, START_TIME_KEY, END_TIME_KEY); - private static final List REQUIRED_NON_NULL_KEYS = Arrays.asList(DURATION_IN_NANOS_KEY); + REQUIRED_NON_EMPTY_KEYS = Arrays.asList(TRACE_ID_KEY, SPAN_ID_KEY, NAME_KEY, KIND_KEY, START_TIME_KEY, END_TIME_KEY); + private static final List REQUIRED_NON_NULL_KEYS = Arrays.asList(DURATION_IN_NANOS_KEY, TRACE_GROUP_FIELDS_KEY); protected JacksonSpan(final Builder builder) { super(builder); @@ -144,10 +148,37 @@ public String getServiceName() { return this.get(SERVICE_NAME_KEY, String.class); } + @Override + public void setTraceGroup(final String traceGroup) { + this.put(TRACE_GROUP_KEY, traceGroup); + } + + @Override + public void setTraceGroupFields(final TraceGroupFields traceGroupFields) { + this.put(TRACE_GROUP_FIELDS_KEY, traceGroupFields); + } + public static Builder builder() { return new Builder(); } + @Override + public String toJsonString() { + final ObjectNode attributesNode = (ObjectNode) getJsonNode().get("attributes"); + final ObjectNode flattenedJsonNode = getJsonNode().deepCopy(); + if (attributesNode != null) { + flattenedJsonNode.remove("attributes"); + for (Iterator> it = attributesNode.fields(); it.hasNext(); ) { + Map.Entry entry = it.next(); + String field = entry.getKey(); + if (!flattenedJsonNode.has(field)) { + flattenedJsonNode.set(field, entry.getValue()); + } + } + } + return flattenedJsonNode.toString(); + } + /** * Builder for creating {@link JacksonSpan} * @since 1.2 @@ -345,33 +376,6 @@ public Builder withServiceName(final String serviceName) { return this; } - /** - * Sets all attributes by copying over those from another span - * @param span - * @since 1.3 - */ - public Builder fromSpan(final Span span) { - this.withSpanId(span.getSpanId()) - .withTraceId(span.getTraceId()) - .withTraceState(span.getTraceState()) - .withParentSpanId(span.getParentSpanId()) - .withName(span.getName()) - .withServiceName(span.getServiceName()) - .withKind(span.getKind()) - .withStartTime(span.getStartTime()) - .withEndTime(span.getEndTime()) - .withAttributes(span.getAttributes()) - .withDroppedAttributesCount(span.getDroppedAttributesCount()) - .withEvents(span.getEvents()) - .withDroppedEventsCount(span.getDroppedEventsCount()) - .withLinks(span.getLinks()) - .withDroppedLinksCount(span.getDroppedLinksCount()) - .withTraceGroup(span.getTraceGroup()) - .withDurationInNanos(span.getDurationInNanos()) - .withTraceGroupFields(span.getTraceGroupFields()); - return this; - } - /** * Returns a newly created {@link JacksonSpan} * @return a JacksonSpan @@ -386,6 +390,10 @@ public JacksonSpan build() { } private void validateParameters() { + REQUIRED_KEYS.forEach(key -> { + checkState(data.containsKey(key), key + " need to be assigned"); + }); + REQUIRED_NON_EMPTY_KEYS.forEach(key -> { final String value = (String) data.get(key); checkNotNull(value, key + " cannot be null"); diff --git a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/Span.java b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/Span.java index 84be1c80ba..3d189a094d 100644 --- a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/Span.java +++ b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/trace/Span.java @@ -139,4 +139,18 @@ public interface Span extends Event { * @since 1.3 */ String getServiceName(); + + /** + * Sets the trace group's name for this span. + * @param traceGroup trace group's name + * @since 1.3 + */ + void setTraceGroup(String traceGroup); + + /** + * Sets the {@link com.amazon.dataprepper.model.trace.TraceGroupFields} for this span. + * @param traceGroupFields trace group related fields + * @since 1.3 + */ + void setTraceGroupFields(TraceGroupFields traceGroupFields); } diff --git a/data-prepper-api/src/test/java/com/amazon/dataprepper/model/trace/DefaultTraceGroupFieldsTest.java b/data-prepper-api/src/test/java/com/amazon/dataprepper/model/trace/DefaultTraceGroupFieldsTest.java index 2ae39fb71b..776454ef37 100644 --- a/data-prepper-api/src/test/java/com/amazon/dataprepper/model/trace/DefaultTraceGroupFieldsTest.java +++ b/data-prepper-api/src/test/java/com/amazon/dataprepper/model/trace/DefaultTraceGroupFieldsTest.java @@ -16,7 +16,6 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; -import static org.junit.jupiter.api.Assertions.assertThrows; public class DefaultTraceGroupFieldsTest { @@ -112,51 +111,4 @@ public void testBuilder_withAllParameters_createsTraceGroupFields() { assertThat(result, is(notNullValue())); } - - @Test - public void testBuilder_withoutParameters_throws_nullPointerException() { - final DefaultTraceGroupFields.Builder builder = DefaultTraceGroupFields.builder(); - assertThrows(NullPointerException.class, builder::build); - } - - @Test - public void testBuilder_throwsNullPointerException_whenEndTimeIsMissing() { - - final DefaultTraceGroupFields.Builder builder = DefaultTraceGroupFields.builder() - .withDurationInNanos(TEST_DURATION) - .withStatusCode(TEST_STATUS_CODE); - - assertThrows(NullPointerException.class, builder::build); - } - - @Test - public void testBuilder_throwsIllegalArgumentException_whenEndTimeIsEmptyString() { - - final DefaultTraceGroupFields.Builder builder = DefaultTraceGroupFields.builder() - .withDurationInNanos(TEST_DURATION) - .withStatusCode(TEST_STATUS_CODE) - .withEndTime(""); - - assertThrows(IllegalArgumentException.class, builder::build); - } - - @Test - public void testBuilder_throwsNullPointerException_whenStatusCodeIsMissing() { - - final DefaultTraceGroupFields.Builder builder = DefaultTraceGroupFields.builder() - .withDurationInNanos(123L) - .withEndTime(TEST_END_TIME); - - assertThrows(NullPointerException.class, builder::build); - } - - @Test - public void testBuilder_throwsNullPointerException_whenDurationIsMissing() { - - final DefaultTraceGroupFields.Builder builder = DefaultTraceGroupFields.builder() - .withStatusCode(200) - .withEndTime(TEST_END_TIME); - - assertThrows(NullPointerException.class, builder::build); - } } diff --git a/data-prepper-api/src/test/java/com/amazon/dataprepper/model/trace/JacksonSpanTest.java b/data-prepper-api/src/test/java/com/amazon/dataprepper/model/trace/JacksonSpanTest.java index 3452ecdf66..497d266655 100644 --- a/data-prepper-api/src/test/java/com/amazon/dataprepper/model/trace/JacksonSpanTest.java +++ b/data-prepper-api/src/test/java/com/amazon/dataprepper/model/trace/JacksonSpanTest.java @@ -6,6 +6,9 @@ package com.amazon.dataprepper.model.trace; import com.amazon.dataprepper.model.event.JacksonEvent; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableMap; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -26,6 +29,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; public class JacksonSpanTest { + private static final ObjectMapper mapper = new ObjectMapper(); private static final String TEST_TRACE_ID = UUID.randomUUID().toString(); private static final String TEST_SPAN_ID = UUID.randomUUID().toString(); @@ -211,6 +215,48 @@ public void testGetTraceGroupFields() { final TraceGroupFields traceGroupFields = jacksonSpan.getTraceGroupFields(); assertThat(traceGroupFields, is(equalTo(traceGroupFields))); } + + @Test + public void testSetAndGetTraceGroup() { + final String testTraceGroup = "testTraceGroup"; + jacksonSpan.setTraceGroup(testTraceGroup); + final String traceGroup = jacksonSpan.getTraceGroup(); + assertThat(traceGroup, is(equalTo(testTraceGroup))); + } + + @Test + public void testSetAndGetTraceGroupFields() { + final TraceGroupFields testTraceGroupFields = DefaultTraceGroupFields.builder() + .withDurationInNanos(200L) + .withStatusCode(404) + .withEndTime("Different end time") + .build(); + jacksonSpan.setTraceGroupFields(testTraceGroupFields); + final TraceGroupFields traceGroupFields = jacksonSpan.getTraceGroupFields(); + assertThat(traceGroupFields, is(equalTo(traceGroupFields))); + assertThat(traceGroupFields, is(equalTo(testTraceGroupFields))); + } + + @Test + public void testToJsonStringAllParameters() throws JsonProcessingException { + final String jsonResult = jacksonSpan.toJsonString(); + final Map resultMap = mapper.readValue(jsonResult, new TypeReference>() {}); + + assertThat(resultMap.containsKey("key1"), is(true)); + assertThat(resultMap.containsKey("key2"), is(true)); + assertThat(resultMap.containsKey("attributes"), is(false)); + } + + @Test + public void testToJsonStringWithoutAttributes() throws JsonProcessingException { + builder.withAttributes(null); + final String jsonResult = builder.build().toJsonString(); + final Map resultMap = mapper.readValue(jsonResult, new TypeReference>() {}); + + assertThat(resultMap.containsKey("key1"), is(false)); + assertThat(resultMap.containsKey("key2"), is(false)); + assertThat(resultMap.containsKey("attributes"), is(false)); + } @Test public void testBuilder_withAllParameters_createsSpan() { @@ -239,53 +285,9 @@ public void testBuilder_withAllParameters_createsSpan() { } @Test - public void testBuilder_fromSpan_createsSpan() { - final JacksonSpan referenceSpan = JacksonSpan.builder() - .withSpanId(TEST_SPAN_ID) - .withTraceId(TEST_TRACE_ID) - .withTraceState(TEST_TRACE_STATE) - .withParentSpanId(TEST_PARENT_SPAN_ID) - .withName(TEST_NAME) - .withServiceName(TEST_SERVICE_NAME) - .withKind(TEST_KIND) - .withStartTime(TEST_START_TIME) - .withEndTime(TEST_END_TIME) - .withAttributes(TEST_ATTRIBUTES) - .withDroppedAttributesCount(TEST_DROPPED_ATTRIBUTES_COUNT) - .withEvents(Arrays.asList(defaultSpanEvent)) - .withDroppedEventsCount(TEST_DROPPED_EVENTS_COUNT) - .withLinks(Arrays.asList(defaultLink)) - .withDroppedLinksCount(TEST_DROPPED_LINKS_COUNT) - .withTraceGroup(TEST_TRACE_GROUP) - .withDurationInNanos(TEST_DURATION_IN_NANOS) - .withTraceGroupFields(defaultTraceGroupFields) - .build(); - final JacksonSpan result = JacksonSpan.builder().fromSpan(referenceSpan).build(); - - assertThat(result, is(notNullValue())); - assertThat(result.getSpanId(), is(equalTo(referenceSpan.getSpanId()))); - assertThat(result.getTraceId(), is(equalTo(referenceSpan.getTraceId()))); - assertThat(result.getTraceState(), is(equalTo(referenceSpan.getTraceState()))); - assertThat(result.getParentSpanId(), is(equalTo(referenceSpan.getParentSpanId()))); - assertThat(result.getName(), is(equalTo(referenceSpan.getName()))); - assertThat(result.getDurationInNanos(), is(equalTo(referenceSpan.getDurationInNanos()))); - assertThat(result.getServiceName(), is(equalTo(referenceSpan.getServiceName()))); - assertThat(result.getKind(), is(equalTo(referenceSpan.getKind()))); - assertThat(result.getStartTime(), is(equalTo(referenceSpan.getStartTime()))); - assertThat(result.getEndTime(), is(equalTo(referenceSpan.getEndTime()))); - assertThat(result.getAttributes(), is(equalTo(referenceSpan.getAttributes()))); - assertThat(result.getDroppedAttributesCount(), is(equalTo(referenceSpan.getDroppedAttributesCount()))); - assertThat(result.getDroppedEventsCount(), is(equalTo(referenceSpan.getDroppedEventsCount()))); - assertThat(result.getDroppedLinksCount(), is(equalTo(referenceSpan.getDroppedLinksCount()))); - assertThat(result.getTraceGroup(), is(equalTo(referenceSpan.getTraceGroup()))); - assertThat(result.getTraceGroupFields(), is(equalTo(referenceSpan.getTraceGroupFields()))); - assertThat(result.getEvents(), is(equalTo(referenceSpan.getEvents()))); - assertThat(result.getLinks(), is(equalTo(referenceSpan.getLinks()))); - } - - @Test - public void testBuilder_withoutParameters_throwsNullPointerException() { - final JacksonEvent.Builder builder = JacksonSpan.builder(); + public void testBuilder_missingNonNullParameters_throwsNullPointerException() { + final JacksonSpan.Builder builder = JacksonSpan.builder(); + builder.withTraceGroup(null); assertThrows(NullPointerException.class, builder::build); } @@ -313,30 +315,6 @@ public void testBuilder_withEmptySpanId_throwsIllegalArgumentException() { assertThrows(IllegalArgumentException.class, builder::build); } - @Test - public void testBuilder_withoutTraceState_throwsNullPointerException() { - builder.withTraceState(null); - assertThrows(NullPointerException.class, builder::build); - } - - @Test - public void testBuilder_withEmptyTraceState_throwsIllegalArgumentException() { - builder.withTraceState(""); - assertThrows(IllegalArgumentException.class, builder::build); - } - - @Test - public void testBuilder_withoutParentSpanId_throwsNullPointerException() { - builder.withParentSpanId(null); - assertThrows(NullPointerException.class, builder::build); - } - - @Test - public void testBuilder_withEmptyParentSpanId_throwsIllegalArgumentException() { - builder.withParentSpanId(""); - assertThrows(IllegalArgumentException.class, builder::build); - } - @Test public void testBuilder_withoutName_throwsNullPointerException() { builder.withName(null); @@ -385,6 +363,35 @@ public void testBuilder_withEmptyEndTime_throwsIllegalArgumentException() { assertThrows(IllegalArgumentException.class, builder::build); } + @Test + public void testBuilder_missingTraceGroupKey_throwsIllegalStateException() { + builder = JacksonSpan.builder() + .withSpanId(TEST_SPAN_ID) + .withTraceId(TEST_TRACE_ID) + .withTraceState(TEST_TRACE_STATE) + .withParentSpanId(TEST_PARENT_SPAN_ID) + .withName(TEST_NAME) + .withServiceName(TEST_SERVICE_NAME) + .withKind(TEST_KIND) + .withStartTime(TEST_START_TIME) + .withEndTime(TEST_END_TIME) + .withAttributes(TEST_ATTRIBUTES) + .withDroppedAttributesCount(TEST_DROPPED_ATTRIBUTES_COUNT) + .withEvents(Arrays.asList(defaultSpanEvent)) + .withDroppedEventsCount(TEST_DROPPED_EVENTS_COUNT) + .withLinks(Arrays.asList(defaultLink)) + .withDroppedLinksCount(TEST_DROPPED_LINKS_COUNT) + .withDurationInNanos(TEST_DURATION_IN_NANOS) + .withTraceGroupFields(defaultTraceGroupFields); + assertThrows(IllegalStateException.class, builder::build); + } + + @Test + public void testBuilder_withoutTraceGroupFields_throwsNullPointerException() { + builder.withTraceGroupFields(null); + assertThrows(NullPointerException.class, builder::build); + } + @Test public void testBuilder_allRequiredParameters_createsSpanWithDefaultValues() { From 5bbfd9007d78b7650f26a028f99594efe03be81d Mon Sep 17 00:00:00 2001 From: Qi Chen <19492223+chenqi0805@users.noreply.github.com> Date: Thu, 17 Mar 2022 15:56:10 -0500 Subject: [PATCH 2/2] STY: unnecessary change of import order Signed-off-by: Qi Chen <19492223+chenqi0805@users.noreply.github.com> --- .../java/com/amazon/dataprepper/model/event/JacksonEvent.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java index 92727962eb..26ab6570e3 100644 --- a/data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java +++ b/data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java @@ -10,8 +10,8 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.ObjectReader; +import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; import com.fasterxml.jackson.databind.type.TypeFactory; import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;