-
Notifications
You must be signed in to change notification settings - Fork 57
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 contextual logger (log.FromContext) #775
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.
This looks like a very interesting approach. Great work. Just wonder if, lets say you have an existing logger created with logger.With() and you want to get a contextual logger from that then this approach wouldn't work. Not sure how common this would be though, but that would be how you would use a contextual logger in core grafana.
But maybe, rather than holding a logger in context holding attributes instead, similar to core grafana https://github.com/grafana/grafana/blob/8ac7736c9561d22789298582eb0cd90e3f789774/pkg/infra/log/log.go#L272-L300. WDYT? Lets discuss
Good point. I think it makes sense to make it similar to core Grafana. I have refactored it so it now stored the attributes in the context rather than the logger directly, and moved log.DefaultLogger.Info("default logger no context", "a", "b")
log.DefaultLogger.FromContext(ctx).Info("default contextual logger", "c", "d")
svcLogger := log.New().With("service", "myservice")
svcLogger.FromContext(ctx).Info("non-default contextual logger", "e", "f") 👇
Regarding |
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 decided to implement the contextual logger in the stream_adapter
as well. We don't have it in core Grafana for some reason:
I think it should also be the case for core Grafana, but maybe I am missing something. If there's a reason behind this decision, I will remove it from here. Otherwise I can also open a PR to add it to streaming methods in core Grafana as well
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.
It does make sense 👍 We should probably have it in core grafana as well
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 awesome work. Just added a comment, but not a blocker for these changes
@@ -12,3 +26,22 @@ var Logger = log.DefaultLogger | |||
var NewLoggerWith = func(args ...interface{}) log.Logger { | |||
return log.New().With(args...) | |||
} | |||
|
|||
func withContextualLogAttributes(ctx context.Context, pCtx PluginContext, endpoint string) context.Context { |
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.
FYI for core plugins using the sdk logger we need to provide these contextual attributes as well together with existing https://github.com/grafana/grafana/blob/50504ba00803a291ea24e8a4a48b14494b97c744/pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go#L59C1-L69 which currently is onyl for the grafana logger :) For now I guess we can duplicate this code to populate attributes for both core grafana logger and sdk logger in core grafana?
Eventually we probably want to include tenant id as well, but we can open a separate issue for that.
What this PR does / why we need it:
Adds a contextual logger, retrievable with
log.FromContext(ctx)
This is a logger that's similar to
log.DefaultLogger
, but it's pre-populated with some contextual parameters:pluginID
endpoint
traceID
(if available)dsName
(if available)dsUID
(if available)uname
(if available)Those are the same attributes logged in Grafana core's contextual logger:
https://github.com/grafana/grafana/blob/50504ba00803a291ea24e8a4a48b14494b97c744/pkg/services/pluginsintegration/clientmiddleware/contextual_logger_middleware.go#L27-L37
For this reason, this is especially useful for decoupling core plugins (replacing contextual logger with plugin sdk contextual logger).
This logger is added to the
context.Context
by the sdk adapter. This is a similar pattern to what happens in Grafana core (logging middleware).Usage example:
Which issue(s) this PR fixes:
Fixes #755
Special notes for your reviewer: