provides first implementation of event models and closes #434 #435

cmanning09 · 2021-10-15T14:37:40Z

Signed-off-by: Christopher Manning [email protected]

Description

Implemented Event and EventMetadata interfaces.
Each implementation contains a builder.
Event implementation uses Jackson Library to manage the underlying data
Provided 100% unit test coverage for each implementation.
Updated Event interface toJsonNode to toJsonString
Added licenses to interfaces.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

dlvenable · 2021-10-15T17:37:19Z

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

+ *
+ * @since 1.2
+ */
+public class JacksonEventImpl implements Event {


I'd like to drop the Impl from this name and make it a JacksonEvent.

dlvenable · 2021-10-15T17:38:01Z

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

+
+    private final ObjectMapper mapper;
+
+    public JacksonEventImpl(final Builder builder) {


This should be private since the Builder creates it.

dlvenable · 2021-10-15T17:43:02Z

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

+        }
+    }
+
+    private String toJsonPointerExpression(final String key) {


It might be nice to use the JsonPointer class here so it is quite clear what it is returning. But, these are private, so it's not critical.

dlvenable · 2021-10-15T17:45:44Z

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

+        this.attributes = builder.attributes == null ? new ImmutableMap.Builder<String, Object>().build() : ImmutableMap.copyOf(builder.attributes);
+    }
+
+    @Override public String getEventType() {


Generally method attributes are put on their own lines.

dlvenable · 2021-10-15T17:46:49Z

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

+ *
+ * @since 1.2
+ */
+public class EventMetadataImpl implements EventMetadata {


Is there another name we can use here? I try to avoid Impl naming in public classes, and even more so for model classes.

I'd suggest DefaultEventMetadata or something like that.

dlvenable · 2021-10-15T17:48:40Z

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

+
+    @Test
+    public void testBuild_withoutEmptyEventType_throwsAnException() {
+        assertThrows(IllegalArgumentException.class, () -> new EventMetadataImpl.Builder().withEventType("").build());


It is best to reduce what is in the assertThrows so that you are sure you are testing the correct method fails.

An exception from either withType() or build() will cause the exception. Which is it that we are validating?

You can extract the Builder into a variable and then reduce it to: assertThrows(IllegalArgumentException.class(), builder.build());

dlvenable · 2021-10-15T18:26:23Z

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

+        assertThrowsForKeyCheck(IllegalArgumentException.class, "bogusKey&");
+    }
+
+    @Test


You can drop the assertThrowsForKeyCheck and consolidate the code using JUnit 5's @ParameterizedTest.

It would look something like this:

@ParameterizedTest @ValueSource(strings = ["", "bogusKey&" /* ... others */]) void testKey_withBadInput_throwsException(String badKey) { assertThrows(expectedThrowable, () -> event.put(badKey, UUID.randomUUID())); assertThrows(expectedThrowable, () -> event.get(badKey, String.class)); assertThrows(expectedThrowable, () -> event.delete(badKey)); }

It will not work for null. You will still need its own test.

I thought about doing this but decided against it because I could make a descriptive test name for each scenario. I can adopt the proposal if this is preferred

I can make the keys descriptive instead so I can still provide context

dlvenable · 2021-10-15T18:33:35Z

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

+
+    public JacksonEventImpl(final Builder builder) {
+
+        this.mapper = new ObjectMapper();


We should probably assign this to a private static ObjectMapper so that we don't need to recreate. Even if we pass it in the future, most instances can still share a common one when it is not injected.

dlvenable · 2021-10-15T18:35:41Z

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

@@ -0,0 +1,117 @@
+package com.amazon.dataprepper.model.event;
+
+import org.junit.Before;


Please use JUnit 5 for all new tests: org.junit.jupiter.api.BeforeEach and org.junit.jupiter.api.Test

laneholloway · 2021-10-15T21:21:26Z

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

+ *
+ * @since 1.2
+ */
+public class EventMetadataImpl implements EventMetadata {


I'd suggest DefaultEventMetadata or something like that.

laneholloway · 2021-10-15T21:23:48Z

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

+        recursivelyTraverseAndPut(jsonNode, keys, value);
+    }
+
+    private void recursivelyTraverseAndPut(final JsonNode node, final LinkedList<String> keys, final Object value) {


While safe for right now, you might want to consider the iterative version of traverse so we won't blow the stack.

That's a good call. That would require us to enforce a limitation on how many nested structures an event would support. I would prefer introducing this sooner rather than later. How many iterations should data prepper support?

Or we could run something indefinitely until there are no more nodes.

I can make it iterative.

I think right now we should be fine without a limit; but if we want to set one, I'd set it to a depth of 100 or something fairly large that people should not be nesting too.

…roject#434 Signed-off-by: Christopher Manning <[email protected]>

Signed-off-by: Christopher Manning <[email protected]>

graytaylor0 · 2021-10-19T17:41:49Z

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

+         * @param eventType the event type
+         * @since 1.2
+         */
+        public Builder withEventType(final String eventType) {


Do we want eventType to be an enum value so that we clearly define the different types of events within Data Prepper?

graytaylor0 · 2021-10-19T17:52:04Z

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

+    private void checkKeyArgument(final String key) {
+        checkNotNull(key, "key cannot be null");
+        checkArgument(!key.isEmpty(), "key cannot be an empty string");
+        checkArgument(key.matches("^([a-zA-Z0-9]([\\w -]*[a-zA-Z0-9])+\\.?)+(?<!\\.)$"), "key must contain only alphanumeric chars with .- and  must follow dot notation (ie. 'field.to.key')");


This implementation concerns me for a couple of reasons.

What about key values that have underscores?

The dot notation is nice, and while it can be the default, we need a way to use the methods here (get, put, delete) with dot notation explicitly turned off. Dot notation does not exist in Logstash (they have a nested syntax of [outer][inner]), but dots are still used in key values fairly often as literal dots, and we should have a boolean option for this model to get keys with dots, and it is likely that I will need to add a configuration option in Grok to enable/disable dot notation.

This is a good question that we need to resolve before we release 1.2.

I wouldn't want to take the [outer][inner] syntax. But, maybe we should consider some of these options:

Use the Jackson JsonPointer syntax of / instead of ..

Support both methods like getPath vs. getKey to differentiate the two.

Other options?

I created this issue to track this: #450

I'm good with approving this PR and we can move the discussion into that ticket.

We can support a translation from the Logstash's nested structure to the dot notation in the converter.

Regarding the keys being able to use dots, I think this is a good call out. I will look to add a way for our users to escape the dot sintax.

cmanning09 requested review from dlvenable and laneholloway October 15, 2021 14:37

dlvenable requested changes Oct 15, 2021

View reviewed changes

dlvenable reviewed Oct 15, 2021

View reviewed changes

laneholloway reviewed Oct 15, 2021

View reviewed changes

cmanning09 added 2 commits October 18, 2021 16:33

provides first implementation of event models and closes opensearch-p…

5d4a333

…roject#434 Signed-off-by: Christopher Manning <[email protected]>

addressing pr feedback

5f94681

Signed-off-by: Christopher Manning <[email protected]>

cmanning09 force-pushed the 434 branch from 413994a to 5f94681 Compare October 18, 2021 21:43

laneholloway approved these changes Oct 18, 2021

View reviewed changes

graytaylor0 reviewed Oct 19, 2021

View reviewed changes

dlvenable approved these changes Oct 19, 2021

View reviewed changes

cmanning09 merged commit 59e3aeb into opensearch-project:main Oct 19, 2021

cmanning09 deleted the 434 branch October 19, 2021 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provides first implementation of event models and closes #434 #435

provides first implementation of event models and closes #434 #435

cmanning09 commented Oct 15, 2021 •

edited

Loading

dlvenable Oct 15, 2021

dlvenable Oct 15, 2021

dlvenable Oct 15, 2021

dlvenable Oct 15, 2021

cmanning09 Oct 18, 2021

dlvenable Oct 15, 2021

laneholloway Oct 15, 2021

dlvenable Oct 15, 2021

dlvenable Oct 15, 2021

cmanning09 Oct 18, 2021

cmanning09 Oct 18, 2021

dlvenable Oct 15, 2021

dlvenable Oct 15, 2021

laneholloway Oct 15, 2021

laneholloway Oct 15, 2021

cmanning09 Oct 18, 2021

cmanning09 Oct 18, 2021

laneholloway Oct 18, 2021

graytaylor0 Oct 19, 2021

graytaylor0 Oct 19, 2021 •

edited

Loading

dlvenable Oct 19, 2021

cmanning09 Oct 19, 2021


		private final ObjectMapper mapper;

		public JacksonEventImpl(final Builder builder) {


		public JacksonEventImpl(final Builder builder) {

		this.mapper = new ObjectMapper();

		@@ -0,0 +1,117 @@
		package com.amazon.dataprepper.model.event;

		import org.junit.Before;

provides first implementation of event models and closes #434 #435

provides first implementation of event models and closes #434 #435

Conversation

cmanning09 commented Oct 15, 2021 • edited Loading

Description

Issues Resolved

Check List

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

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

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

graytaylor0 Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmanning09 commented Oct 15, 2021 •

edited

Loading

graytaylor0 Oct 19, 2021 •

edited

Loading