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 null and ignore_missing check to handle event.original field. #8624

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

ritalwar
Copy link
Contributor

@ritalwar ritalwar commented Dec 1, 2023

Proposed commit message

This PR add null and ignore_missing check to handle event.original field in multiple packages which are as follows:

  • ActiveMQ
  • CoreDns
  • IBMMQ
  • MongoDB
  • Salesforce
  • Spring_Boot
  • vSphere

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Related issues

Screenshots

@ritalwar ritalwar added the bugfix Pull request that fixes a bug issue label Dec 1, 2023
@ritalwar ritalwar requested a review from a team as a code owner December 1, 2023 04:59
@ritalwar ritalwar marked this pull request as draft December 1, 2023 04:59
@elasticmachine
Copy link

elasticmachine commented Dec 1, 2023

💚 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: 2023-12-01T09:03:58.907+0000

  • Duration: 20 min 9 sec

Test stats 🧪

Test Results
Failed 0
Passed 159
Skipped 0
Total 159

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Dec 1, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (16/16) 💚
Files 100.0% (19/19) 💚 15.385
Classes 100.0% (19/19) 💚 15.385
Methods 98.19% (217/221) 👍 8.36
Lines 94.12% (2529/2687) 👍 4.905
Conditionals 100.0% (0/0) 💚

@ritalwar ritalwar marked this pull request as ready for review December 1, 2023 13:55
@@ -15,7 +15,7 @@ processors:
field: event.original
value: "{{{message}}}"
ignore_empty_value: true
Copy link
Contributor

Choose a reason for hiding this comment

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

@ritalwar - As we have the ignore_missing check in most places. Can we adopt the same option here as well and replace the ignore_empty_value with ignore_missing option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@muthu-mps In the Rename processor, we use ignore_missing, while in the Set processor, ignore_empty_value serves the same purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for clarity, We will have RENAME and SET processors for integrations. These are defined previously and will be maintained as it is and both the processor does the same function.

Copy link
Contributor Author

@ritalwar ritalwar Dec 4, 2023

Choose a reason for hiding this comment

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

The only difference between SET and RENAME in this scenario is that with SET, we end up having two fields, namely message and event.original. Ideally, we should eliminate the message field after Set. However, it should apply to other such fields as well, and the removal of the message field is not being addressed as part of this PR.

@muthu-mps muthu-mps self-requested a review December 4, 2023 09:00
Copy link
Contributor

@muthu-mps muthu-mps left a comment

Choose a reason for hiding this comment

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

LGTM!

@ishleenk17
Copy link
Contributor

@ritalwar : In some plavces there were set processors, in some rename. Whats the general guidance you have followed here ?

@ritalwar
Copy link
Contributor Author

ritalwar commented Dec 5, 2023

@ritalwar : In some plavces there were set processors, in some rename. Whats the general guidance you have followed here ?

I haven't modified any processors; I've maintained the set and rename operations as they were and simply introduced these checks.

@ishleenk17
Copy link
Contributor

@ritalwar : In some plavces there were set processors, in some rename. Whats the general guidance you have followed here ?

I haven't modified any processors; I've maintained the set and rename operations as they were and simply introduced these checks.

Ok, looks good!

@ritalwar ritalwar merged commit c28d057 into elastic:main Dec 5, 2023
4 checks passed
@elasticmachine
Copy link

Package activemq - 0.14.1 containing this change is available at https://epr.elastic.co/search?package=activemq

@elasticmachine
Copy link

Package coredns - 0.6.1 containing this change is available at https://epr.elastic.co/search?package=coredns

@elasticmachine
Copy link

Package ibmmq - 1.2.4 containing this change is available at https://epr.elastic.co/search?package=ibmmq

@elasticmachine
Copy link

Package mongodb - 1.12.1 containing this change is available at https://epr.elastic.co/search?package=mongodb

@elasticmachine
Copy link

Package salesforce - 0.12.1 containing this change is available at https://epr.elastic.co/search?package=salesforce

@elasticmachine
Copy link

Package spring_boot - 1.2.1 containing this change is available at https://epr.elastic.co/search?package=spring_boot

@elasticmachine
Copy link

Package vsphere - 1.9.2 containing this change is available at https://epr.elastic.co/search?package=vsphere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants