-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
x-pack/filebeat/input/cel: log transaction ID in CEL evaluations #37065
Conversation
1925fda
to
6bd5182
Compare
Emit a previous transaction ID before starting CEL evaluation and the final ID after completing the work. Also write transaction IDs to CEL debug log call if available.
6bd5182
to
c018dec
Compare
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.
Nits
@@ -167,6 +167,15 @@ func (rt *LoggingRoundTripper) RoundTrip(req *http.Request) (*http.Response, err | |||
return resp, err | |||
} | |||
|
|||
// TxID returns the current transaction.id value. If rt is nil, the empty string is returned. | |||
func (rt *LoggingRoundTripper) TxID() string { |
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.
Possible to add test cases for this func?
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.
What failure modes are you expecting that can't be proven safe by inspection?
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.
Not just about failure scenarios. There is a specific format that is being returned here, so just thought if it is covered in a test to guard against unsafe changes in future. But you can take a call here. This might not change often or at all.
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 I'm OK with what we have; the two places that return a transaction ID are within 10 lines of each other and are very clearly related. I did think about making them tied, but didn't see a good way to do it. I have that now, so I'll make that change.
…stic#37065) Emit a previous transaction ID before starting CEL evaluation and the final ID after completing the work. Also write transaction IDs to CEL debug log call if available.
Proposed commit message
Emit a previous transaction ID before starting CEL evaluation and the final ID after completing the work. Also write transaction IDs to CEL debug log call if available.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Run
go test -v -run TestInput/tracer_filename_sanitization
in the cel package to see transaction IDs in the log.Related issues
Use cases
Screenshots
Logs