Conversation
|
@grobinson-grafana for your comment here, do you have a preference what key name we align on? |
Group key is the correct name. |
|
Ping, any additional work needed here? |
tjhop
force-pushed
the
chore/logging-fixes
branch
from
1a116a1 to
1c3ab9a
Compare
Addresses feedback from another PR prometheus#4089 (comment) Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
This changes the majority of our `Notify()` implementations to set up a new logger with the group key attached under the key name `group_key`, and then to use that logger in all subsequent calls to the logger, including passing it through to further functions that accept loggers via params. A few of the notify implementations are more complicated; they either extract the key later in their `Notify()` implementation or within sub methods, or even conditionally like with sns. I left those mostly as is for now, as they seem to be more snow-flake-y. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
tjhop
force-pushed
the
chore/logging-fixes
branch
from
1c3ab9a to
a64bfe2
Compare
|
Sorry for the noise here -- I needed to resolve a conflict and accidentally rebased on my copy of main instead of upstream. This is all cleaned up and ready for review now 👍 |
|
|
||
| n.logger.Debug("extracted group key", "key", key) | ||
| logger := n.logger.With("group_key", key) | ||
| logger.Debug("extracted group key") |
There was a problem hiding this comment.
Shall we move this inside notify.ExtractGroupKey() instead of repeating the same line of code in all integrations?
There was a problem hiding this comment.
Sounds reasonable, but I'm trying to think through the most sane way to do that. Currently, notify.ExtractGroupKey() has the following signature:
Lines 159 to 166 in 29d491e
Do we want to update the function signature to accept a logger as well?
Alternatively, I could use the top level slog.Debug() function inside of notify.ExtractGroupKey() if we want to shim a call in to main somewhere after we initialize the logger to slog.SetDefault(logger).
Thoughts?
There was a problem hiding this comment.
I'd say skip it, not worth the effort.
There was a problem hiding this comment.
Agreed. Still a good point, maybe a better option will present in the future
4 participants
Some logging fixes to address PR feedback from #4089; correct error levels in a few spots and improve consistency.