Pass FromContext(...) context to slog handler#425
Conversation
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
…thContext Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
| // slog.LevelInfo and V(4) becomes slog.LevelDebug. | ||
| // | ||
| // The provided context is passed to the slog.Handler when logging. This allows | ||
| // the slog.Handler to use the context. This should only be used for short-lived |
There was a problem hiding this comment.
Would the following a little more descriptive?
an slog which following the lifecycle of the context
not limit to the vague long lived v.s. short lived
There was a problem hiding this comment.
Could you provide a diff? I would like to understand exactly what you would like to change, as I think that the current comment is quite clear already.
|
following the context of #158 context of logger is not really helpful if we are going to take the information in context for log events with current implementation, we have to create logger for those events. |
| // The provided context is passed to the slog.Handler when logging. This allows | ||
| // the slog.Handler to use the context. This should only be used for short-lived | ||
| // Loggers that do not outlive the context. If you need a long-lived Logger, use | ||
| // FromSlogHandler instead. |
There was a problem hiding this comment.
| // FromSlogHandler instead. | |
| // FromSlogHandler instead or | |
| // pass context.Background(). |
pohly
left a comment
pohly
left a comment
There was a problem hiding this comment.
I'm not sure how useful this is. It only provides a benefit in a very limited scenario (slog handler is used which supports reading values from a context, log calls must be rewritten to inject the context via the new FromSlogHandlerWithContext). Does using logr provide any value at all in this scenario?
What about logr.FromContext? Should that automatically use FromSlogHandlerWithContext? That could make a difference because then apps don't need to be updated. However, we then can easily end up with creating loggers which outlive their original context and log the wrong values from that context later on, so this wouldn't be reliable.
cc @thockin
4 participants
Instead of passing
context.Background(), we now pass the context passed toFromContext(...)to the slog.Handle(...)function instead.This is an alternative solution to the problem presented in #158 (only solves the use case where an slog backend is used).
cc @pohly