Skip to content

[Azure Logs] Handle event.original field from upstream forwarders #3639

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

Merged
merged 2 commits into from
Jul 8, 2022

Conversation

zmoog
Copy link
Contributor

@zmoog zmoog commented Jul 1, 2022

What does this PR do?

With this PR, the Azure Logs pipelines will use the event.original field over the message field when it exists in the actual event.

Upstream event forwarders like Logstash can add their event.original field, depending on the circumstances (specific
configurations or tool versions), causing issues like #3636.

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

Upstream event forwarders like Logstash can add their
`event.original` field, depending on the circumstances (specific
configurations or tool versions).

With the change, the pipeline will honor the event.original field over
the message field, when it exists.
@zmoog zmoog added the enhancement New feature or request label Jul 1, 2022
@zmoog zmoog self-assigned this Jul 1, 2022
@zmoog zmoog changed the title [Azure Logs] Handle event.original from upstream forwarders [Azure Logs] Handle event.original field from upstream forwarders Jul 1, 2022
@zmoog
Copy link
Contributor Author

zmoog commented Jul 1, 2022

Note for reviewers: I'm not a pipeline expert, and I'd love to hear from you if there are better ways to achieve the same goal.

@zmoog zmoog added bug Something isn't working, use only for issues Team:Cloud-Monitoring Label for the Cloud Monitoring team and removed enhancement New feature or request labels Jul 1, 2022
@elasticmachine
Copy link

elasticmachine commented Jul 1, 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-07-01T16:10:33.969+0000

  • Duration: 15 min 54 sec

Test stats 🧪

Test Results
Failed 0
Passed 89
Skipped 0
Total 89

🤖 GitHub comments

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

  • /test : Re-trigger the build.

@elasticmachine
Copy link

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (6/6) 💚
Files 78.571% (11/14) 👎 -17.956
Classes 78.571% (11/14) 👎 -17.956
Methods 77.876% (88/113) 👎 -11.489
Lines 81.855% (1606/1962) 👎 -9.088
Conditionals 100.0% (0/0) 💚

@zmoog
Copy link
Contributor Author

zmoog commented Jul 1, 2022

Another note for the reviewers: I'd also love to add a test for this change, but the test infrastructure focuses on the message, and AFAIK it does not offer options to customize event fields. Any advice about how to archive this?

@zmoog zmoog marked this pull request as ready for review July 1, 2022 16:23
@zmoog zmoog requested a review from a team as a code owner July 1, 2022 16:23
@legoguy1000
Copy link
Contributor

This seems like this would be an issue for every integration of logstash adds an event.original field

@kaiyan-sheng
Copy link
Contributor

Agree with @legoguy1000 and I think the fix should be on logstash side.

@tommyers-elastic
Copy link
Contributor

@kaiyan-sheng i agree this fix should be made on the logstash side, but I think it's reasonable to add this mitigation here as well. especially since we can release the integration independently and ahead of logstash. the only consideration is if this change has any performance impact on the pipeline (it looks like it would be pretty minimal to me)?

the logstash issue to track the fix over there is here

Copy link
Contributor

@tommyers-elastic tommyers-elastic left a comment

Choose a reason for hiding this comment

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

these changes look good to me - thanks @zmoog. just one question regarding performance, any idea if the addition of these conditionals come with any measurable perf hit?

@tommyers-elastic
Copy link
Contributor

after following up on the perf element, we do not have anything to worry about here.

@tommyers-elastic tommyers-elastic merged commit f61483b into elastic:main Jul 8, 2022
@zmoog
Copy link
Contributor Author

zmoog commented Jul 18, 2022

after following up on the perf element, we do not have anything to worry about here.

This sounds good; I'll compare this change's performance impact with the previous version to ensure it doesn't move the needle in the wrong direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working, use only for issues Team:Cloud-Monitoring Label for the Cloud Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Azure Logs] Ingestion fails when event.original is present
5 participants