-
Notifications
You must be signed in to change notification settings - Fork 444
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
[cloudflare_logpush] Fix logic on http_request rename processors #8629
[cloudflare_logpush] Fix logic on http_request rename processors #8629
Conversation
- Fixed incorrect if statements on rename processors for fields that were renamed by Cloudflare. - If both fields were present, then the second rename processor would try to run when it should've been skipped. Rename processors cannot rename a field to one that already exists.
🚀 Benchmarks reportPackage
|
Data stream | Previous EPS | New EPS | Diff (%) | Result |
---|---|---|---|---|
nel_report |
25641.03 | 16666.67 | -8974.36 (-35%) | 💔 |
To see the full report comment with /test benchmark fullreport
🌐 Coverage report
|
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a test case to demonstrate the issue.
Edit: Never mind, I must've been looking at the wrong file by mistake, we do test these fields. I had originally renamed the old fields to the new. I'll re-add the old fields alongside the new, which will test this case. |
@@ -510,31 +510,31 @@ processors: | |||
value: '{{{_ingest.on_failure_message}}}' | |||
- rename: | |||
field: json.SecurityActions | |||
target_field: cloudflare_logpush.http_request.firewall.match.action | |||
target_field: cloudflare_logpush.http_request.firewall.matches.action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why the names are being changed as well. This is not explained in the commit messages or issue AFAICS. Can this detail be added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add this to the commit message. This was a bug with the original implementation. The field definitions did not match those in the pipeline. This was never caught before because we didn't have those fields in the test data (which I've added now).
Package cloudflare_logpush - 1.17.3 containing this change is available at https://epr.elastic.co/search?package=cloudflare_logpush |
Proposed commit message
Checklist
changelog.yml
file.Related issues