Capture log params from context metadata #1049
Conversation
wbrowne
requested review from
andresmgot,
oshirohugo and
xnyo
and removed request for
a team
I wanted to keep alongside what we have now since it's virtually the same thing except for the difference in context types. Plus the tenant package is in Have added some tests! |
|
|
||
| // WithContextualAttributesForOutgoingContext will append the given key/value log parameters to the outgoing context. | ||
| func WithContextualAttributesForOutgoingContext(ctx context.Context, logParams []any) context.Context { | ||
| if len(logParams) == 0 || len(logParams)%2 != 0 { |
There was a problem hiding this comment.
it could be confusing to do nothing if by mistake someone calls this with an uneven set of logParams. What if len(logParams)%2 != 0 you add an empty param at the end? (That's how the logger works, right?)
There was a problem hiding this comment.
Yeah I can see that it's quite unforgiving, but I'm aiming for simplicity here.
you add an empty param at the end? (That's how the logger works, right?)
Usually what I see is the EXTRA_VALUE_AT_END - which can be a bit strange/unexpected. Plus we can't know if we're missing specifically a key or a value, so fudging things to work can also end up in a weird state.
There was a problem hiding this comment.
yeah, the problem is that if someone hits this, it would be difficult to know what's going on since no error or anything is returned but up to you if you want to leave that for later.
| var attrs []any | ||
| for _, param := range logParams { | ||
| kv := strings.Split(param, logParamSeparator) | ||
| if len(kv) != 2 || kv[0] == "" || kv[1] == "" { |
There was a problem hiding this comment.
an empty value is not a valid value?
There was a problem hiding this comment.
Yeah I wanted to keep things simple for now. I don't foresee this being used heavily as it's quite a special use case. Can always revisit if it becomes important
There was a problem hiding this comment.
I think it would be useful to see if a value is set but it's empty but maybe I am missing something so up to you.
|
|
||
| var attrs []any | ||
| for _, param := range logParams { | ||
| kv := strings.Split(param, logParamSeparator) |
There was a problem hiding this comment.
it may be needed to scape the logParamSeparator in case a param includes : on it?
There was a problem hiding this comment.
Great catch. I just updated it to be 3 characters instead 👀 rather than making things more complex. WDYT?
There was a problem hiding this comment.
I guess that's fine unless the parameter is a ipv6 😅 I was more thinking about replacing : with \: but it can be left for later.
|
|
||
| // WithContextualAttributesForOutgoingContext will append the given key/value log parameters to the outgoing context. | ||
| func WithContextualAttributesForOutgoingContext(ctx context.Context, logParams []any) context.Context { | ||
| if len(logParams) == 0 || len(logParams)%2 != 0 { |
There was a problem hiding this comment.
yeah, the problem is that if someone hits this, it would be difficult to know what's going on since no error or anything is returned but up to you if you want to leave that for later.
|
|
||
| var attrs []any | ||
| for _, param := range logParams { | ||
| kv := strings.Split(param, logParamSeparator) |
There was a problem hiding this comment.
I guess that's fine unless the parameter is a ipv6 😅 I was more thinking about replacing : with \: but it can be left for later.
| var attrs []any | ||
| for _, param := range logParams { | ||
| kv := strings.Split(param, logParamSeparator) | ||
| if len(kv) != 2 || kv[0] == "" || kv[1] == "" { |
There was a problem hiding this comment.
I think it would be useful to see if a value is set but it's empty but maybe I am missing something so up to you.
What this PR does / why we need it:
Similar to existing functionality in
backend/log/context.go, we need a way to pass + retrieve contextual log information via outgoing / incoming context. This gives us the ability for the client side to dictate useful log parameters.For example:
{"@level":"debug","@message":"Plugin Request Completed","@timestamp":"2024-08-15T14:30:53.382034+01:00","dsName":"GitHub","dsUID":"a5219f2a-6a14-48bd-b986-7a7822573bcc","duration":"3.306700834s","endpoint":"queryData","foo":"bar","pluginID":"grafana-github-datasource","stackId":"321","stackSlug":"wbrowne","status":"ok","statusSource":"plugin","uname":"admin"}Which issue(s) this PR fixes:
Fixes #1040
Special notes for your reviewer: