Added default timestamp to JacksonEvent #943

asifsmohammed · 2022-01-26T20:29:56Z

Description

Added @timestamp key to JacksonEvent wtih a custom Serializer for ZonedDateTime
Fixed failing Unit and Integration tests because of new key.

Issue related to Serializaing DateTIme using Jackson
FasterXML/jackson-modules-java8#76

Issues Resolved

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

asifsmohammed · 2022-01-26T20:35:26Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java

@@ -79,6 +87,9 @@ protected JacksonEvent(final Builder builder) {
        }

        this.jsonNode = getInitialJsonNode(builder.data);
+
+        zonedDateTime = ZonedDateTime.now().format(DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSXXX"));


I want to point out that I used DateTimeFormatter here because if we don't have it trailing zeros will be removed from timestamp during serialization as seen in below example. And it will return String type instead of ZonedDateTime object.

expected :{\"event_timestamp\":\"2022-01-26T13:35:32.270-06:00\"} Actual :{\"event_timestamp\":\"2022-01-26T13:35:32.27-06:00\"}

dlvenable · 2022-01-26T21:26:44Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java

@@ -66,6 +72,8 @@

    static final String EVENT_TYPE = "event";

+    private static String zonedDateTime;


You shouldn't need this field.

dlvenable · 2022-01-26T21:27:31Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java

@@ -88,6 +99,10 @@ static Event fromMessage(String message) {
                .build();
    }

+    static String getZonedDateTime() {


I don't believe that we want this.

There could be value in having a getEventTimestamp() field on Event. But, that would be different. And I believe that it should pull from the Map, not have a duplicated field.

dlvenable · 2022-01-26T21:36:28Z

data-prepper-api/src/test/java/com/amazon/dataprepper/model/event/JacksonEventTest.java

    }

    @Test
    public void testGetAsMap_with_EmptyData() {
        final Map<String, Object> eventAsMap = event.toMap();
-        assertThat(eventAsMap, equalTo(Collections.emptyMap()));
+
+        final Map<String, Object> mapObject = new HashMap<>();


I'd probably rename this to something like expectedMap.

dlvenable · 2022-01-26T21:37:05Z

data-prepper-api/src/test/java/com/amazon/dataprepper/model/event/JacksonEventTest.java

-        assertThat(eventAsMap, equalTo(Collections.emptyMap()));
+
+        final Map<String, Object> mapObject = new HashMap<>();
+        mapObject.put("event_timestamp", JacksonEvent.getZonedDateTime());


You should create your ZonedDateTime in this test. It can be in either the setUp or in this method. You don't need that static method on JacksonEvent.

dlvenable · 2022-01-26T21:38:41Z

data-prepper-api/src/test/java/com/amazon/dataprepper/model/event/JacksonEventTest.java

@@ -299,7 +301,7 @@ public void testKey_withNullKey_throwsNullPointerException() {
    public void testToString_withEmptyData() {
        final String result = event.toJsonString();

-        assertThat(result, is(equalTo("{}")));
+        assertThat(result, is(equalTo("{\"event_timestamp\":\"" + JacksonEvent.getZonedDateTime() + "\"}")));


I'd call get("event_timestamp") on the new Event.

Then you can compare against that instead of this static method.

dlvenable · 2022-01-26T21:39:06Z

data-prepper-api/src/test/java/com/amazon/dataprepper/model/event/JacksonEventTest.java

@@ -310,13 +312,21 @@ public void testToString_withSimpleObject() {
        event.put("list", Arrays.asList(1, 4, 5));
        final String result = event.toJsonString();

-        assertThat(result, is(equalTo(String.format("{\"foo\":\"bar\",\"testObject\":{\"field1\":\"%s\"},\"list\":[1,4,5]}", value))));
+        String expectedResult = "{\"event_timestamp\":\"" + JacksonEvent.getZonedDateTime() +


Similar comment on calling get("event_timestamp") and comparing against that.

asifsmohammed · 2022-01-27T02:13:19Z

I think there is an issue with either Jackson or Java Date Time according to this open issue.
FasterXML/jackson-modules-java8#76

When we serialize a Date Time object, trailing zeros in milliseconds will be trimmed and jsonNode will have "2022-01-26T19:35:26.80307-06:00" as timestamp.
When we deserialize it again as part of get method, zeros will be appended back.
"2022-01-26T19:35:26.803070-06:00"

So whenever we try to use get method to compare json string or map asserts will fail in unit tests.

The workaround for this is to use DateTimeFormatter which will return a string containing trailing zeros.

dlvenable · 2022-01-27T14:50:49Z

I think there is an issue with either Jackson or Java Date Time according to this open issue. FasterXML/jackson-modules-java8#76

When we serialize a Date Time object, trailing zeros in milliseconds will be trimmed and jsonNode will have "2022-01-26T19:35:26.80307-06:00" as timestamp. When we deserialize it again as part of get method, zeros will be appended back. "2022-01-26T19:35:26.803070-06:00"

So whenever we try to use get method to compare json string or map asserts will fail in unit tests.

The workaround for this is to use DateTimeFormatter which will return a string containing trailing zeros.

@asifsmohammed,

That is a disappointing bug. Using DataTimeFormatter in Jackson will probably require a custom serializer. If this is easily configured in the ObjectMapper that is fine.

An alternative is to change the tests to not test for string equality. Instead you can compare two ZonedDateTime objects and trim out the milliseconds.

Here is an example:

public void testToString_withEmptyData() {
    final String result = event.toJsonString();

    // Original comparison
    //assertThat(result, is(equalTo("{\"event_timestamp\":\"" + JacksonEvent.getZonedDateTime() + "\"}")));
    assertThat(result, notNullValue());
    
    ZonedDateTime actualTrimmedEventTimestamp = event.get("event_timestamp").truncatedTo(ChronoUnit.SECONDS);

    Map deserializedJson = objectMapper.readValue(result, Map.class);
    assertThat(deserializedJson, hasKey("event_timestamp"));
    ZonedDateTime trimmedJsonTimestamp = ((ZonedDateTime)deserializedJson.get("event_timestamp")).truncatedTo(ChronoUnit.SECONDS)
    assertThat(trimmedJsonTimestamp, equalTo(actualTrimmedEventTimestamp));

cmanning09 · 2022-01-27T14:57:09Z

Please link to the issue in the PR description.

graytaylor0 · 2022-01-27T17:37:19Z

data-prepper-api/src/test/java/com/amazon/dataprepper/model/event/JacksonEventTest.java

+    @Test
+    public void testTimestamp_isNotNullAndValid() {
+        Assertions.assertNotNull(timestamp);
+        Assertions.assertTrue(Duration.between(timestamp, OffsetDateTime.now()).getSeconds() < 1);


Is there ever a chance that all the tests take more than one second to run before this assertion is made? If so this could result in a flaky test. It may make more sense to make the timestamp here right before the assertion

Good catch. But, the timestamp is not created separately, we are getting it from the event which will be created for each test. So technically it shouldn't take more than 1 second.
I'll try to increase it to 5 seconds just so that test won't be flaky.

This is another reason why I like createObjectUnderTest() methods instead of creating it in setUp().

Instant before = Instant.now(); Event objectUnderTest = createObjectUnderTest(); Instant after = Instant.now(); assertThat(objectUnderTest.get("event_timestamp"), lessThanOrEqualTo(after)); assertThat(objectUnderTest.get("event_timestamp"), greaterThanOrEqualTo(before));

graytaylor0 · 2022-01-27T17:52:54Z

The build is failing due to some tests in the AggregateActions that have comparison being done between a Map and Event.toMap(), and Event.toMap() has the event_timestamp added. As it may break tests that compare two Events as well, I think it may be less damaging if the event_timestamp is only added as a part of the Event on serialization, or to at least exclude the event_timestamp from impacting the comparison of two events.

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

codecov-commenter · 2022-01-31T02:09:25Z

Codecov Report

Merging #943 (8b2ad58) into main (fdd92b9) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #943      +/-   ##
============================================
+ Coverage     91.40%   91.44%   +0.03%     
- Complexity      673      675       +2     
============================================
  Files            83       84       +1     
  Lines          1955     1964       +9     
  Branches        165      165              
============================================
+ Hits           1787     1796       +9     
  Misses          129      129              
  Partials         39       39

Impacted Files	Coverage Δ
...nedDateTimeSerializerWithMilliSecondPrecision.java	`100.00% <100.00%> (ø)`
...m/amazon/dataprepper/model/event/JacksonEvent.java	`99.23% <100.00%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdd92b9...8b2ad58. Read the comment docs.

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

cmanning09 · 2022-01-31T18:57:13Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/event/JacksonEvent.java

@@ -79,6 +89,7 @@ protected JacksonEvent(final Builder builder) {
        }

        this.jsonNode = getInitialJsonNode(builder.data);
+        put(TIMESTAMP_KEY, ZonedDateTime.now());


What is the user experience if the @timestamp already existed in the event? It looks like the exsiting @timestamp be overwritten.

I think we should update the JacksonEvent docs to indicate a @timestamp field will be added to the object and the expected default behavior for an existing value.

I'll update the Java docs for JacksonEvent.

I am not sure if overwriting the field is the correct thing to do here. This has the potential to erase customer data and impact peer forwarding.

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

asifsmohammed requested a review from a team as a code owner January 26, 2022 20:29

asifsmohammed commented Jan 26, 2022

View reviewed changes

dlvenable requested changes Jan 26, 2022

View reviewed changes

graytaylor0 reviewed Jan 27, 2022

View reviewed changes

asifsmohammed added 3 commits January 30, 2022 20:01

Added event timestamp to Jackson log

375d898

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

Added timestamp to JacksonEvent

e3de5db

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

Added custom serializer for OffsetDateTime

667f6aa

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

asifsmohammed force-pushed the timestamp branch from 9191fb5 to b9c0d09 Compare January 31, 2022 02:01

asifsmohammed force-pushed the timestamp branch from b9c0d09 to e03be1a Compare January 31, 2022 02:31

Added @timestamp to event and updated Unit and Integration tests

8b2ad58

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

asifsmohammed force-pushed the timestamp branch from e03be1a to 8b2ad58 Compare January 31, 2022 05:21

cmanning09 reviewed Jan 31, 2022

View reviewed changes

Updated Java doc for JacksonEvent

3e35574

Signed-off-by: Asif Sohail Mohammed <nsifmoh@amazon.com>

asifsmohammed closed this Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added default timestamp to JacksonEvent #943

Added default timestamp to JacksonEvent #943

asifsmohammed commented Jan 26, 2022 •

edited

asifsmohammed Jan 26, 2022 •

edited

dlvenable Jan 26, 2022

dlvenable Jan 26, 2022

dlvenable Jan 26, 2022

dlvenable Jan 26, 2022

dlvenable Jan 26, 2022

dlvenable Jan 26, 2022

asifsmohammed commented Jan 27, 2022

dlvenable commented Jan 27, 2022

cmanning09 commented Jan 27, 2022

graytaylor0 Jan 27, 2022

asifsmohammed Jan 27, 2022 •

edited

dlvenable Jan 27, 2022

graytaylor0 commented Jan 27, 2022 •

edited

codecov-commenter commented Jan 31, 2022 •

edited

cmanning09 Jan 31, 2022

asifsmohammed Jan 31, 2022

cmanning09 Feb 1, 2022

		@@ -66,6 +72,8 @@

		static final String EVENT_TYPE = "event";

		private static String zonedDateTime;

Added default timestamp to JacksonEvent #943

Added default timestamp to JacksonEvent #943

Conversation

asifsmohammed commented Jan 26, 2022 • edited

Description

Issues Resolved

Check List

asifsmohammed Jan 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asifsmohammed commented Jan 27, 2022

dlvenable commented Jan 27, 2022

cmanning09 commented Jan 27, 2022

Choose a reason for hiding this comment

asifsmohammed Jan 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graytaylor0 commented Jan 27, 2022 • edited

codecov-commenter commented Jan 31, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asifsmohammed commented Jan 26, 2022 •

edited

asifsmohammed Jan 26, 2022 •

edited

asifsmohammed Jan 27, 2022 •

edited

graytaylor0 commented Jan 27, 2022 •

edited

codecov-commenter commented Jan 31, 2022 •

edited