Modify EventHandle to be created for every event and support internal and external origination times #3546

kkondaka · 2023-10-24T23:02:21Z

Description

Modify EventHandle to be created for every event and support internal and external origination times.

Moved DefaultEvent handle to data-prepper-api from data-prepper-core
Added API to get/set acknowledgementset, and get/set external origination times
EventHandle is now created by default with internal origination time when JacksonEvent is created

Partially resolves #3494

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

[ X] New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
[X ] 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.

… and external origination times Signed-off-by: Krishna Kondaka <[email protected]>

Signed-off-by: Krishna Kondaka <[email protected]>

graytaylor0 · 2023-10-27T15:04:02Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/DefaultEventHandle.java

+
+    @Override
+    public void release(boolean result) {
+        System.out.println("======release called==="+result);


Should remove this print statement

graytaylor0 · 2023-10-27T15:04:52Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/JacksonEvent.java

    }

    public static Event fromMessage(String message) {
-        return JacksonEvent.builder()
+        JacksonEvent event = JacksonEvent.builder()


What does this change accomplish?

Not needed. I was trying something. undid the change. Thanks!

graytaylor0 · 2023-10-27T15:05:36Z

...ntegrationTest/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSinkIT.java

+        /*
        lenient().doAnswer(a -> {
            return null;
        }).when(eventHandle).release(any(Boolean.class));
+        */


Extra comment

Please delete commented out code.

graytaylor0 · 2023-10-27T15:05:53Z

...gins/s3-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/s3/S3SinkServiceTest.java

        s3SinkService.output(records);

-        final List<EventHandle> eventHandles = records.stream().map(Record::getData).map(Event::getEventHandle).collect(Collectors.toList());
+        //final List<EventHandle> eventHandles = records.stream().map(Record::getData).map(Event::getEventHandle).collect(Collectors.toList());


Extra comment

graytaylor0 · 2023-10-27T15:08:50Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/DefaultEventHandle.java

+        return this.internalOriginationTime;
+    }
+
+    @Override


For the next PR let's scope down and first only implement internal end to end latency metric. For ddb use case we could potentially use this value for external (https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_streams_StreamRecord.html#DDB-Type-streams_StreamRecord-ApproximateCreationDateTime), so we may not need to make any hasty decisions on users configuring this time. @dlvenable What do you think?

If I understand your proposal, the Source will set this value on the Event. We don't need to let the user configure this in the pipeline. But, we could in the future. Right?

OK. Will only implement internal latency for now in the OpenSearchSink and S3 Sink

dlvenable · 2023-10-27T19:00:16Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/DefaultEventHandle.java

+import java.time.Instant;
+import java.io.Serializable;
+
+public class DefaultEventHandle implements EventHandle, Serializable {


Can we make this package protected? I'd really like to avoid this getting used as it will be hard to clean-up in the future.

By "package protected" you mean make this as
protected class DefaultEventHandle implements EventHandle, Serializable?

dlvenable · 2023-10-27T19:01:05Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/EventHandle.java

+     * @param acknowledgementSet acknowledgementSet to be set in the event handle
+     * @since 2.6
+     */
+    void setAcknowledgementSet(final AcknowledgementSet acknowledgementSet);


Do we need to expose all of this? It might make sense to have a different interface for internal use. Say InternalEventHandle. Then DefaultEventHandle can implement that.

Not sure I understand this suggestion. Are you suggesting we have InternalEventHandle and EventHandle separately?

Yes, we'd have two interfaces:

public interface InterfaceEventHandle { ... // methods that data-prepper-core calls are here. }

And we implement it as:

class DefaultEventHandle implements EventHandle, InternalEventHandle { ... }

In data-prepper, core we'd need a cast: InternalEventHandle internalEventHandle = (InternalEventHandle) eventHandle.

This would be a good indication that somebody is doing something they shouldn't if we see it outside of data-prepper-core.

dlvenable · 2023-10-27T19:02:14Z

...ntegrationTest/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSinkIT.java

+        /*
        lenient().doAnswer(a -> {
            return null;
        }).when(eventHandle).release(any(Boolean.class));
+        */


Please delete commented out code.

dlvenable · 2023-10-27T19:02:36Z

...gins/s3-sink/src/test/java/org/opensearch/dataprepper/plugins/sink/s3/S3SinkServiceTest.java

+        s3SinkService.output(records);
+
+        for (EventHandle eventHandle : eventHandles) {
+            System.out.println("==2====EventHandle=="+eventHandle+"==="+acknowledgementSet);


Please remove println usage.

dlvenable · 2023-10-27T19:04:48Z

data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/EventHandle.java

+     * @param externalOriginationTime externalOriginationTime to be set in the event handle
+     * @since 2.6
+     */
+    void setExternalOriginationTime(final Instant externalOriginationTime);


I think we should set the origination time on the EventMetadata. The EventHandle can then get this data from the EventMetadata.

Internal origination time is set in EventMetadata and EventHandle gets it from it. Do you want the external origination time also set in the EventMetadata?

Yes, add this to EventMetadata.

The EventHandle can also hold a reference to the time in there.

Signed-off-by: Krishna Kondaka <[email protected]>

opensearch-trigger-bot · 2023-11-05T23:42:04Z

The backport to 2.5 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.5 2.5
# Navigate to the new working tree
cd .worktrees/backport-2.5
# Create a new branch
git switch --create backport/backport-3546-to-2.5
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7869eb7aac41965adc9a3eab4f85d4121f7b41b9
# Push it to GitHub
git push --set-upstream origin backport/backport-3546-to-2.5
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.5

Then, create a pull request where the base branch is 2.5 and the compare/head branch is backport/backport-3546-to-2.5.

kkondaka requested review from chenqi0805, engechas, graytaylor0, dinujoh, asifsmohammed, dlvenable and oeyh as code owners October 24, 2023 23:02

kkondaka force-pushed the event-handle-changes branch 2 times, most recently from 0e382c6 to 310bb2e Compare October 25, 2023 22:19

Krishna Kondaka added 3 commits October 25, 2023 22:21

Modify EventHandle to be created for every event and support internal…

26db9db

… and external origination times Signed-off-by: Krishna Kondaka <[email protected]>

Fixed build failures

3cbef88

Signed-off-by: Krishna Kondaka <[email protected]>

Fixed build failures

5e144bd

Signed-off-by: Krishna Kondaka <[email protected]>

kkondaka force-pushed the event-handle-changes branch from 310bb2e to 5e144bd Compare October 25, 2023 22:21

Krishna Kondaka added 2 commits October 25, 2023 23:31

fixed failing checkstyle error

680bf65

Signed-off-by: Krishna Kondaka <[email protected]>

Fixed build errors

eeedfb8

Signed-off-by: Krishna Kondaka <[email protected]>

graytaylor0 reviewed Oct 27, 2023

View reviewed changes

dlvenable requested changes Oct 27, 2023

View reviewed changes

Addressed review comments by adding InternalEventHandle

a9c3ebd

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable previously approved these changes Nov 1, 2023

View reviewed changes

graytaylor0 previously approved these changes Nov 1, 2023

View reviewed changes

Fixed checkstyle errors

53b1787

Signed-off-by: Krishna Kondaka <[email protected]>

kkondaka dismissed stale reviews from graytaylor0 and dlvenable via 53b1787 November 1, 2023 18:12

Fixed build errors

df44704

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable approved these changes Nov 1, 2023

View reviewed changes

graytaylor0 approved these changes Nov 1, 2023

View reviewed changes

kkondaka merged commit 7869eb7 into opensearch-project:main Nov 1, 2023
57 checks passed

asifsmohammed added the backport 2.5 label Nov 5, 2023

dlvenable mentioned this pull request Nov 6, 2023

Add Internal and external latency to OpenSearch and S3 sinks. #3583

Merged

2 tasks

kkondaka deleted the event-handle-changes branch May 13, 2024 05:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify EventHandle to be created for every event and support internal and external origination times #3546

Modify EventHandle to be created for every event and support internal and external origination times #3546

kkondaka commented Oct 24, 2023

graytaylor0 Oct 27, 2023

kkondaka Oct 27, 2023

graytaylor0 Oct 27, 2023

kkondaka Oct 27, 2023

graytaylor0 Oct 27, 2023

dlvenable Oct 27, 2023

graytaylor0 Oct 27, 2023

graytaylor0 Oct 27, 2023

dlvenable Oct 27, 2023

kkondaka Oct 27, 2023

dlvenable Oct 27, 2023

kkondaka Oct 27, 2023

dlvenable Oct 27, 2023

kkondaka Oct 27, 2023

dlvenable Oct 27, 2023

dlvenable Oct 27, 2023

dlvenable Oct 27, 2023

dlvenable Oct 27, 2023

kkondaka Oct 27, 2023

dlvenable Oct 27, 2023

opensearch-trigger-bot bot commented Nov 5, 2023

Modify EventHandle to be created for every event and support internal and external origination times #3546

Modify EventHandle to be created for every event and support internal and external origination times #3546

Conversation

kkondaka commented Oct 24, 2023

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

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Nov 5, 2023