Skip to content
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

Logging: Add WithContextualAttributes to pass log params based on the given context #74428

Merged
merged 9 commits into from Sep 7, 2023

Conversation

svennergr
Copy link
Contributor

@svennergr svennergr commented Sep 6, 2023

What is this feature?

This PR adds WithContextualAttributes method, that will just register some labels/log-parameters on the given context and return a new context with those values. If a new logger is created with logger.FromContext, those parameters will automatically be prepended.

This is especially useful for datasource loggers to also include the endpoint, dsName, dsUID and pluginId.

@svennergr svennergr self-assigned this Sep 6, 2023
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.2.x milestone Sep 6, 2023
@svennergr svennergr changed the title [WIP] Logging: Suggestion to use RegisterContextualLogProvider [WIP] Logging: Suggestion to use RegisterContextualLogProvider to pass log params to data source loggers Sep 6, 2023
pkg/infra/log/log.go Outdated Show resolved Hide resolved
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Sorry I probably answered a bit confusing earlier. I'll respond in the existing review thread

@@ -146,6 +149,14 @@ func NewClientDecorator(
c := client.ProvideService(pluginRegistry, pCfg)
middlewares := CreateMiddlewares(cfg, oAuthTokenService, tracer, cachingService, features)

log.RegisterContextualLogProvider(func(ctx context.Context) ([]any, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Since WithContextualAttributes is in the log package I would recommend putting this in there as well/together with it in the same package and by so you don't need to expose log.LogParamsContextKey.

@@ -123,4 +130,9 @@ func reverseMiddlewares(middlewares []plugins.ClientMiddleware) []plugins.Client
return reversed
}

func instrumentContext(ctx context.Context, endpoint string, pCtx backend.PluginContext) context.Context {
Copy link
Member

Choose a reason for hiding this comment

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

With the approach you did choose I think you can keep this in instrumentation.go for now as you had initially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With that I needed to change the function signature in e.g. InstrumentCallResourceRequest to also contain the context:https://github.com/grafana/grafana/pull/74428/files#diff-f68ce913764de9b2ec9c118f29476e2aeec3981d7561124e48659e5a8c2c6f78L111

Let me know what you think.

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM with some nits

pkg/infra/log/log.go Show resolved Hide resolved
@svennergr svennergr marked this pull request as ready for review September 7, 2023 10:01
@svennergr svennergr requested review from a team as code owners September 7, 2023 10:01
@svennergr svennergr requested review from xnyo, papagian, zserge and suntala and removed request for a team September 7, 2023 10:01
@svennergr svennergr changed the title [WIP] Logging: Suggestion to use RegisterContextualLogProvider to pass log params to data source loggers Logging: Add WithContextualAttributes to pass log params based on the given context Sep 7, 2023
@svennergr svennergr added add to changelog no-backport Skip backport of PR labels Sep 7, 2023
Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

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

Yay for this! Great job @svennergr for tackling this at backend plugin level rather than us having to pass these to every log individually. 🚀

@svennergr svennergr merged commit 7e01dde into main Sep 7, 2023
13 checks passed
@svennergr svennergr deleted the svennergr/contextual-ds-logging branch September 7, 2023 11:13
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
…he given context (grafana#74428)

* suggestion to use `RegisterContextualLogProvider`

* add `pluginId`

* change to `WithContextualAttributes`

* move approach to instrumentation.go

* improve `WithContextualAttributes`

* unexport consts

* typo

* remove comment

* add `nil` check
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
…he given context (#74428)

* suggestion to use `RegisterContextualLogProvider`

* add `pluginId`

* change to `WithContextualAttributes`

* move approach to instrumentation.go

* improve `WithContextualAttributes`

* unexport consts

* typo

* remove comment

* add `nil` check
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants