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

[Enhancement][libbeat] Add append processor #33364

Merged
merged 15 commits into from
Nov 17, 2022

Conversation

Vinit023
Copy link

@Vinit023 Vinit023 commented Oct 16, 2022

Type of change

  • Enhancement

What does this PR do?

This PR here is to add the append processor in the libbeat. using this processor users will be able to append values from either a field or static values to either an existing or a new target key in the response.

The code will get 3 parameters target_field, fields and values. And It will append the values of each field listed under the fields section and then it will append all the values listed under the values key.
If the target_key is an existing key with values, It will be appended in an array first.

Why is it important?

As a user I will be able to append values to a field using beats processors.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@cla-checker-service
Copy link

cla-checker-service bot commented Oct 16, 2022

💚 CLA has been signed

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 16, 2022
@botelastic
Copy link

botelastic bot commented Oct 16, 2022

This pull request doesn't have a Team:<team> label.

@mergify
Copy link
Contributor

mergify bot commented Oct 16, 2022

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @WinIT23? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 16, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-11-16T20:07:42.266+0000

  • Duration: 77 min 14 sec

Test stats 🧪

Test Results
Failed 0
Passed 24107
Skipped 1951
Total 26058

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@Vinit023 Vinit023 force-pushed the add-append-processor branch from 79197aa to 020214a Compare October 16, 2022 22:50
@Vinit023 Vinit023 changed the title Add append processor [Enhancement][libbeat] Add append processor Oct 16, 2022
@Vinit023 Vinit023 marked this pull request as ready for review October 16, 2022 23:08
@Vinit023 Vinit023 requested a review from a team as a code owner October 16, 2022 23:08
@Vinit023 Vinit023 requested review from faec and leehinman and removed request for a team October 16, 2022 23:08
if err != nil {
f.logger.Debugf("could not fetch value for key: %s. all the values will be appended in a new key %s.", target, target)
} else {
arr = append(arr, val)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check the type of val here? I'm thinking we need to do the same as line 117 below.

Either way, it would be nice to add a test where target already exists as both a scalar and a list.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think a type check is required. as the event.GetValue returns interface{} and we will have to append if it's a scaler. We just have to iterate in case of a list. Hance the type check on L117.

Let me know if you think otherwise. 🤔

Copy link
Author

Choose a reason for hiding this comment

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

You were right. I have added a type check in this line to avoid appending and array inside an array.

<titleabbrev>append_processor</titleabbrev>
++++

The `append_processor` processor appends static values and values from the listed fields to target field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand description so it is like the append ingest node processor? I think we want to behavior of these two append implementations to be the same.

https://www.elastic.co/guide/en/elasticsearch/reference/current/append-processor.html

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I have that particular processor in mind while developing this one. I'll update the description in this one though.

PS. could you please let me know if I'm missing any other doc file? CHANGELOG or any other? I'm unaware of the doc files that needed to be changed for libbeat. 😅

@Vinit023 Vinit023 requested review from leehinman and removed request for faec October 19, 2022 19:03
Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

Looks good.

Just needs an entry in CHANGELOG.next.asciidoc in the Added section for all beats.

@Vinit023
Copy link
Author

Hey @leehinman - Added a changelog entry let me know if anything else is required. 😄

@Vinit023 Vinit023 requested a review from leehinman November 15, 2022 16:15
@leehinman leehinman merged commit edc4e07 into elastic:main Nov 17, 2022
@mr1716
Copy link
Contributor

mr1716 commented Nov 17, 2022

Will this also be added into Filebeat so Filebeat users can implement it?

@Vinit023
Copy link
Author

Hey @mr1716 - It will be accessible in all the Beats. So yes, you'll be able to use it in Filebeat.

@mr1716
Copy link
Contributor

mr1716 commented Jan 12, 2023

@vinit-chauhan thanks. I dont see the documentation for the append processor here: https://www.elastic.co/guide/en/beats/filebeat/current/filtering-and-enhancing-data.html Does that need to be updated to reflect this new processor?

@Vinit023
Copy link
Author

Hey @mr1716 - it is available in the main branch docs here https://www.elastic.co/guide/en/beats/filebeat/master/append.html

@leehinman can you or anyone from your team confirm if the change would be part of 8.7.0? moreover, can we backport it to 8.6.0?

@mr1716
Copy link
Contributor

mr1716 commented Jan 12, 2023

@vinit-chauhan Thanks/ Much appreciated. It says that it is a part of all beats for 8.6. So if it isnt, there might need to be a change in the release notes as it isnt there for filebeat.

@Vinit023
Copy link
Author

Hey @mr1716 you are right, for some reason code is not part of 8.6.0 and I'm not sure why?! I believe someone from elastic team might be able to answer it better.

@leehinman
Copy link
Contributor

@leehinman can you or anyone from your team confirm if the change would be part of 8.7.0? moreover, can we backport it to 8.6.0?

The merge didn't happen before feature freeze for 8.6.0, so that is why it isn't there. It will be part of 8.7.0.

@mr1716
Copy link
Contributor

mr1716 commented Jan 12, 2023

@leehinman can you or anyone from your team confirm if the change would be part of 8.7.0? moreover, can we backport it to 8.6.0?

The merge didn't happen before feature freeze for 8.6.0, so that is why it isn't there. It will be part of 8.7.0.

@leehinman If that's the case, the document should be updated accordingly

chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
Add append processor that matches the behavior of the elasticsearch ingest node append processor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs_team Indicates that the issue/PR needs a Team:* label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Filebeat] [Processors] Add Append Processor
5 participants