chore!: adopt log/slog, remove go-kit/log by tjhop · Pull Request #14906 · prometheus/prometheus (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation48 Commits1 Checks25 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
For: #14355
This commit updates Prometheus to adopt stdlib's log/slog package in
favor of go-kit/log. As part of converting to use slog, several other
related changes are required to get prometheus working, including:
- removed unused logging util func
RateLimit() - forward ported the util/logging/Deduper logging by implementing a small custom slog.Handler that does the deduping before chaining log calls to the underlying real slog.Logger
- move some of the json file logging functionality to use prom/common package functionality
- refactored some of the new json file logging for scraping
- changes to promql.QueryLogger interface to swap out logging methods for relevant slog sugar wrappers
- updated lots of tests that used/replicated custom logging functionality, attempting to keep the logical goal of the tests consistent after the transition
- added a healthy amount of
if logger == nil { $makeLogger }type conditional checks amongst various functions where none were provided -- old code that used the go-kit/log.Logger interface had several places where there were nil references when trying to use functions likeWith()to add keyvals on the new *slog.Logger type
@SuperQ if you don't mind taking a look when you have time as well, I'd appreciate it!
And sorry all for the massive change set 😅
Opening as draft for now so it's known that there's work in progress -- still need to fix up tests around the custom query log testing that's done in cmd/prometheus/query_log_test.go, but I believe that should cover most things.
It builds and has been running well for me in my testing so far; feedback welcome!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sterling stuff! Thanks for doing this; some initial comments below.
(I only read about 25% of the PR, so far)
Comment on lines -624 to -637
| // Above level 6, the k8s client would log bearer tokens in clear-text. |
|---|
| klog.ClampLevel(6) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seem to have lost this line.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Above level 6, the k8s client would log bearer tokens in clear-text. |
|---|
| klog.ClampLevel(6) |
| klog.SetLogger(log.With(logger, "component", "k8s_client_runtime")) |
| klogv2.ClampLevel(6) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same response as the other one -- though I do need to double check and see if/where we still need klog v1 in prometheus to see about creating SetSlogLogger() type redirect func like there is in klogv2 upstream
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, we still have v1 things in discovery, I took inspiration from klog's upstream v1/v2 coexistence docs and implemented a small wrapper to redirect klogv1 calls to klogv2, and since klogv2 is already redirected to log/slog, that should mean both flavors of klog end up in slog in the end
| log.With(logger, "component", "scrape manager"), |
|---|
| func(s string) (log.Logger, error) { return logging.NewJSONFileLogger(s) }, |
| logger.With("component", "scrape manager"), |
| func(s string) (*logging.JSONFileLogger, error) { return logging.NewJSONFileLogger(s) }, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return a more abstract interface type?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tjhop added a commit to tjhop/common that referenced this pull request
Simple convenience function to return an slog.Logger that writes to io.Discard. Originally suggested by @ArthurSens [here](prometheus#686 (comment)), and requested again by @bboreham [here](prometheus/prometheus#14906 (comment)). As Bryan points out in the comment, there's 147 instances where a discard logger is needed, so a consistent utility function to manage them seems helpful.
Signed-off-by: TJ Hoplock t.hoplock@gmail.com
@bboreham I believe this is ready for a re-review. I've rebased on current upstream main, incorporated fixes for some of the feedback provided so far, and go tests working locally.
Things work locally using go workspaces, but as I mention in the updated PR description, this requires functionality in 2 PRs that haven't made their way to an upstream release yet in prometheus/common. Because of this, test failures should be expected as things fail to compile for obvious reasons.
SuperQ pushed a commit to prometheus/common that referenced this pull request
Simple convenience function to return an slog.Logger that writes to io.Discard. Originally suggested by @ArthurSens [here](#686 (comment)), and requested again by @bboreham [here](prometheus/prometheus#14906 (comment)). As Bryan points out in the comment, there's 147 instances where a discard logger is needed, so a consistent utility function to manage them seems helpful.
Signed-off-by: TJ Hoplock t.hoplock@gmail.com
Pin to a commit in go.mod ?
Pin to a commit in
go.mod?
Sure, I can do that. I'd prefer not to do that at merge time, but happy to do that for now at least.
Bumped prometheus/common to v0.60.0, rebased to solve a merge conflict, squashed commits, and updated commit description to match current state 👍
I approve this but I would appreciate if we do not have the full path to source file
time=2024-10-01T16:54:36.703+02:00 level=INFO source=/home/roidelapluie/dev/prometheus/cmd/prometheus/main.go:1363 msg="Notifier manager stopped"
didn't we go for ts ?
Needs a rebase, but I will merge asap once this is fixed
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh, I see slog does not return error, but we sill moved query logger to slog?
Mmh, I see slog does not return error, but we sill moved query logger to slog?
Yes, so far we've been operating with the goal of "full slog adoption" as much as possible. The QueryLogger interface has been updated to match the slog sugar logger methods available, the difference being that With() on the query logger does not return a new logger but rather updates the querylogger's internal logger.
I had some similar thoughts. Do you have any thoughts/opinions?
I would have prefered not to change the QueryLogger and ScrapeFailure loggers but no strong opinion
I'm game for merge. Like @SuperQ had mentioned a ways above, let's get it in and iterate on improvements 👍
@roidelapluie As far as I remember looking at the go-kit/log code, the Log() interface swallowed all errors anyway and always returned nil. I think it was only there for internal use or something like that. This is why we added an exception to golangci-lint configs to ignore errcheck issues.
Which reminds me, @tjhop , you can remove this from the .golanci.yml
# Never check for logger errors.
- (github.com/go-kit/log.Logger).Log
Which reminds me, @tjhop , you can remove this from the .golanci.yml
Woops, I had it in there at one point, must've lost in one of the recent rebases. Thanks!
SuperQ previously approved these changes Oct 7, 2024
Did a thorough self-review and see a few other spots to tidy up, will get things better cleaned up
For: prometheus#14355
This commit updates Prometheus to adopt stdlib's log/slog package in favor of go-kit/log. As part of converting to use slog, several other related changes are required to get prometheus working, including:
- removed unused logging util func
RateLimit() - forward ported the util/logging/Deduper logging by implementing a small custom slog.Handler that does the deduping before chaining log calls to the underlying real slog.Logger
- move some of the json file logging functionality to use prom/common package functionality
- refactored some of the new json file logging for scraping
- changes to promql.QueryLogger interface to swap out logging methods for relevant slog sugar wrappers
- updated lots of tests that used/replicated custom logging functionality, attempting to keep the logical goal of the tests consistent after the transition
- added a healthy amount of
if logger == nil { $makeLogger }type conditional checks amongst various functions where none were provided -- old code that used the go-kit/log.Logger interface had several places where there were nil references when trying to use functions likeWith()to add keyvals on the new *slog.Logger type
Signed-off-by: TJ Hoplock t.hoplock@gmail.com
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets gooooooooooooooooo
tjhop added a commit to tjhop/prometheus that referenced this pull request
Resolves: prometheus#15433
When I converted prometheus to use slog in prometheus#14906, I update both the
QueryLogger interface, as well as how the log calls to the
QueryLogger were built up in promql.Engine.exec(). The backing
logger for the QueryLogger in the engine is a
util/logging.JSONFileLogger, and it's implementation of the With()
method updates the logger the logger in place with the new keyvals added
onto the underlying slog.Logger, which means they get inherited onto
everything after. All subsequent calls to With(), even in later
queries, would continue to then append on more and more keyvals for the
various params and fields built up in the logger. In turn, this causes
unbounded growth of the logger, leading to increased memory usage, and
in at least one report was the likely cause of an OOM kill. More
information can be found in the issue and the linked slack thread.
This commit does a few things:
- It was referenced in feedback in prometheus#14906 that it would've been better
to not change the
QueryLoggerinterface if possible, this PR proposes changes that bring it closer to alignment with the pre-3.0QueryLoggerinterface contract - reverts
promql.Engine.exec()'s usage of the query logger to the pattern of building up an array of args to pass at once to the end log call. Avoiding the repetitious calls to.With()are what resolve the issue with the logger growth/memory usage. - updates the scrape failure logger to use the update
QueryLoggermethods in the contract. - updates tests accordingly
- cleans up unused methods
Builds and passes tests successfully. Tested locally and confirmed I could no longer reproduce the issue/it resolved the issue.
Signed-off-by: TJ Hoplock t.hoplock@gmail.com
tjhop added a commit to tjhop/prometheus that referenced this pull request
Resolves: prometheus#15433
When I converted prometheus to use slog in prometheus#14906, I update both the
QueryLogger interface, as well as how the log calls to the
QueryLogger were built up in promql.Engine.exec(). The backing
logger for the QueryLogger in the engine is a
util/logging.JSONFileLogger, and it's implementation of the With()
method updates the logger the logger in place with the new keyvals added
onto the underlying slog.Logger, which means they get inherited onto
everything after. All subsequent calls to With(), even in later
queries, would continue to then append on more and more keyvals for the
various params and fields built up in the logger. In turn, this causes
unbounded growth of the logger, leading to increased memory usage, and
in at least one report was the likely cause of an OOM kill. More
information can be found in the issue and the linked slack thread.
This commit does a few things:
- It was referenced in feedback in prometheus#14906 that it would've been better
to not change the
QueryLoggerinterface if possible, this PR proposes changes that bring it closer to alignment with the pre-3.0QueryLoggerinterface contract - reverts
promql.Engine.exec()'s usage of the query logger to the pattern of building up an array of args to pass at once to the end log call. Avoiding the repetitious calls to.With()are what resolve the issue with the logger growth/memory usage. - updates the scrape failure logger to use the update
QueryLoggermethods in the contract. - updates tests accordingly
- cleans up unused methods
Builds and passes tests successfully. Tested locally and confirmed I could no longer reproduce the issue/it resolved the issue.
Signed-off-by: TJ Hoplock t.hoplock@gmail.com
julienduchesne pushed a commit to julienduchesne/prometheus that referenced this pull request
Signed-off-by: Arve Knudsen arve.knudsen@gmail.com
Reviewers
