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.

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

Add append option to add_entries processor #4143

Merged
merged 3 commits into from
Feb 20, 2024

Conversation

oeyh
Copy link
Collaborator

@oeyh oeyh commented Feb 16, 2024

Description

Adds an append_if_key_exists option to add_entries processor as described in #4129

Issues Resolved

Resolves #4129

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.

Signed-off-by: Hai Yan <[email protected]>
@graytaylor0
Copy link
Member

Would it make sense to instead have something like this

action_when_key_exists: append //  can be one of `append`, `overwrite`, `skip`

Not a blocking comment just something to consider

graytaylor0
graytaylor0 previously approved these changes Feb 19, 2024
@oeyh
Copy link
Collaborator Author

oeyh commented Feb 19, 2024

action_when_key_exists: append // can be one of append, overwrite, skip

@graytaylor0 I like it, making the config more concise. I can do the change if other reviewers think the same.

public String getAddWhen() { return addWhen; }

@AssertTrue(message = "Either value or format or expression must be specified, and only one of them can be specified")
public boolean hasValueOrFormatOrExpression() {
return Stream.of(value, format, valueExpression).filter(n -> n!=null).count() == 1;
}

@AssertTrue(message = "overwrite_if_key_exists and append_if_key_exists can not be set at the same time.")
Copy link
Member

Choose a reason for hiding this comment

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

overwrite_if_key_exists and append_if_key_exists can not be set at the same time.

Please change to:

overwrite_if_key_exists and append_if_key_exists can not be set to true at the same time.

They can be "set" together.

@@ -97,4 +101,30 @@ public boolean isReadyForShutdown() {
@Override
public void shutdown() {
}

private void mergeValueToEvent(final Event recordEvent, final String key, final Object value) {
Copy link
Member

Choose a reason for hiding this comment

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

Consider combining these with the use of Java 8 lambdas.

Here is some starting point for one way to do it.

private void mergeValueToEvent(final Event recordEvent, final String key, final Object value)
  mergeValue(recordEvent, () -> recordEvent.getMetadata().getAttribute(key), newValue -> recordEvent.put(key, newValue);
}

mergeValue(Supplier<Object> getter, Consumer<Object> setter) {
          final Object currentValue = getter.get();
        final List<Object> mergedValue = new ArrayList<>();
        if (currentValue instanceof List) {
            mergedValue.addAll((List<Object>) currentValue);
        } else {
            mergedValue.add(currentValue);
        }

        mergedValue.add(value);
        setter.accept(mergedValue);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion!

@dlvenable
Copy link
Member

Would it make sense to instead have something like this

action_when_key_exists: append //  can be one of `append`, `overwrite`, `skip`

Not a blocking comment just something to consider

@graytaylor0 , I like this idea. But, I also think we should aim for consistency. Right now grok has overwrite_when_key_exists. Do you think grok could also move toward using this approach? I'd really like to see all the processors use similar behavior. And I think this enum could facilitate that.

@oeyh
Copy link
Collaborator Author

oeyh commented Feb 20, 2024

These processors have similar overwrite_if_key_exists options:

  • copy_values
  • rename_keys
  • parse_json
  • parse_ion
  • key_value

grok processor has a keys_to_overwrite option so that you can determine to overwrite or append (default) for each grok generated field.

I will open a follow-up issue for these and we can discuss further there.

@oeyh oeyh merged commit 94e9e65 into opensearch-project:main Feb 20, 2024
46 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Append values to lists in an event
3 participants