Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updates the user_agent processor to use the EventKey. #4628

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Updates the user_agent processor to use the EventKey. #4628

merged 4 commits into from
Jul 9, 2024

Conversation

dlvenable
Copy link
Member

Description

This draft PR updates the user_agent processor to use the EventKey. It depends on changes from #4627, so this is a draft PR for now.

Issues Resolved

N/A

Check List

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

@dlvenable dlvenable mentioned this pull request Jun 14, 2024
Merged
4 tasks
@dlvenable dlvenable force-pushed the 1916-event-key-user-agent-processor branch from a903eec to 70fd9fc Compare June 17, 2024 15:57
@dlvenable dlvenable marked this pull request as ready for review June 17, 2024 16:04
@dlvenable
Copy link
Member Author

Now that #4627 is merged, I've rebased this PR and force-pushed it. So this is ready for review and merge.

@dlvenable dlvenable mentioned this pull request Jun 20, 2024
Open
19 tasks
@dlvenable
Updates the user_agent processor to use the EventKey.
a47e8aa 
Signed-off-by: David Venable <dlv@amazon.com>
@dlvenable dlvenable force-pushed the 1916-event-key-user-agent-processor branch from 70fd9fc to a47e8aa Compare June 20, 2024 20:07
@dlvenable
Use an EventKeyConfiguration in the user_agent source property.
57dba9b 
Signed-off-by: David Venable <dlv@amazon.com>
@dlvenable
Copy link
Member Author

I've updated this PR to use the EventKeyConfiguration provided in #4635 for the source field. The target is a little trickier to use with the configuration since it has a default value. But, both are using the EventKey - just through different means.

KarstenSchnitter
KarstenSchnitter reviewed Jun 27, 2024
View reviewed changes
Copy link
Collaborator

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing the change. I am not quite sure about the purpose of the refactoring. So far, It looks to me, as if no change in behaviour is made.

...src/main/java/org/opensearch/dataprepper/plugins/processor/useragent/UserAgentProcessor.java Outdated Show resolved Hide resolved
...src/main/java/org/opensearch/dataprepper/plugins/processor/useragent/UserAgentProcessor.java Outdated Show resolved Hide resolved
...src/main/java/org/opensearch/dataprepper/plugins/processor/useragent/UserAgentProcessor.java Outdated
} catch (Exception e) {
LOG.error(EVENT, "An exception occurred when parsing user agent data from event [{}] with source key [{}]",
event, config.getSource(), e);
event, sourceKey.getKey(), e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intended to only log the key part after the change? To be compatible with the previous version, this would need to be just sourceKey.

@dlvenable @KarstenSchnitter
Apply suggestions from code review
ecf3789 
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Signed-off-by: David Venable <dlv@amazon.com>
@dlvenable dlvenable force-pushed the 1916-event-key-user-agent-processor branch from 1ee8acd to ecf3789 Compare June 27, 2024 18:26
@dlvenable
Changes to use only sourceKey as noted in the PR review.
a162646 
Signed-off-by: David Venable <dlv@amazon.com>
@dlvenable
Copy link
Member Author

Thanks for providing the change. I am not quite sure about the purpose of the refactoring. So far, It looks to me, as if no change in behaviour is made.

@KarstenSchnitter , Thanks for the suggestions. I took all of them.

I'm making this change because of some of the problems we have with the current approach. In particular: 1) The original approach uses a lot of processing to build the JSON Pointers on-demand. 2) The original approach did not validate keys up-front. This meant that users would create invalid configs and only know it after data is flowing in.

This new approach lets us create the EventKey when constructing the config. This moves the validation of bad configs further up. And it also lets us re-use the existing internal constructs (e.g. JSON Pointer).

@dlvenable dlvenable merged commit 1d259cf into opensearch-project:main Jul 9, 2024
46 of 47 checks passed
@dlvenable dlvenable mentioned this pull request Jul 18, 2024
Merged
4 tasks
kkondaka pushed a commit to kkondaka/kk-data-prepper-f2 that referenced this pull request Jul 23, 2024
@dlvenable @KarstenSchnitter
Updates the user_agent processor to use the EventKey. (opensearch-pro…
6793499 
…ject#4628)

Updates the user_agent processor to use the EventKey.

Signed-off-by: David Venable <dlv@amazon.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
kkondaka pushed a commit to kkondaka/kk-data-prepper-f2 that referenced this pull request Jul 23, 2024
@dlvenable @KarstenSchnitter
Updates the user_agent processor to use the EventKey. (opensearch-pro…
ebed23d 
…ject#4628)

Updates the user_agent processor to use the EventKey.

Signed-off-by: David Venable <dlv@amazon.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
kkondaka pushed a commit to kkondaka/kk-data-prepper-f2 that referenced this pull request Jul 30, 2024
@dlvenable @KarstenSchnitter
Updates the user_agent processor to use the EventKey. (opensearch-pro…
4642f14 
…ject#4628)

Updates the user_agent processor to use the EventKey.

Signed-off-by: David Venable <dlv@amazon.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
kkondaka pushed a commit to kkondaka/kk-data-prepper-f2 that referenced this pull request Aug 8, 2024
@dlvenable @KarstenSchnitter
Updates the user_agent processor to use the EventKey. (opensearch-pro…
15142ba 
…ject#4628)

Updates the user_agent processor to use the EventKey.

Signed-off-by: David Venable <dlv@amazon.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
kkondaka pushed a commit to kkondaka/kk-data-prepper-f2 that referenced this pull request Aug 12, 2024
@dlvenable @KarstenSchnitter
Updates the user_agent processor to use the EventKey. (opensearch-pro…
e7c56f2 
…ject#4628)

Updates the user_agent processor to use the EventKey.

Signed-off-by: David Venable <dlv@amazon.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
kkondaka pushed a commit to kkondaka/kk-data-prepper-f2 that referenced this pull request Aug 14, 2024
@dlvenable @KarstenSchnitter
Updates the user_agent processor to use the EventKey. (opensearch-pro…
84e812b 
…ject#4628)

Updates the user_agent processor to use the EventKey.

Signed-off-by: David Venable <dlv@amazon.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
@dlvenable dlvenable deleted the 1916-event-key-user-agent-processor branch October 22, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KarstenSchnitter KarstenSchnitter KarstenSchnitter left review comments

@kkondaka kkondaka kkondaka approved these changes

@oeyh oeyh oeyh approved these changes

@chenqi0805 chenqi0805 Awaiting requested review from chenqi0805 chenqi0805 is a code owner

@engechas engechas Awaiting requested review from engechas engechas is a code owner

@graytaylor0 graytaylor0 Awaiting requested review from graytaylor0 graytaylor0 is a code owner

@dinujoh dinujoh Awaiting requested review from dinujoh dinujoh is a code owner

@asifsmohammed asifsmohammed Awaiting requested review from asifsmohammed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dlvenable @KarstenSchnitter @oeyh @kkondaka