-
Notifications
You must be signed in to change notification settings - Fork 704
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
Log correlation for operator APM traces #5883
Conversation
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.
Haven't finished review yet. Couple questions from start of review.
@@ -107,7 +107,7 @@ func buildOutputConfig(params Params) (*settings.CanonicalConfig, error) { | |||
return settings.NewCanonicalConfig(), nil | |||
} | |||
|
|||
credentials, err := association.ElasticsearchAuthSettings(params.Client, assoc) | |||
credentials, err := association.ElasticsearchAuthSettings(params.Context, params.Client, assoc) |
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 exactly related to this PR, but why are we storing context?
https://pkg.go.dev/context#pkg-overview
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:
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 guess because the struct here is just a parameter object but I agree it is maybe not ideal.
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.
lgtm.
Add log correlation to APM tracing. This replaces global log variables with loggers retrieved from a context object which carries the necessary transaction and span information for correlation.
Add log correlation to APM tracing. This replaces global log variables with loggers retrieved from a context object which carries the necessary transaction and span information for correlation.