ref!: adopt log/slog + other prom configs, drop custom logging pkg #1586
Conversation
This file contains 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
I would call this a breaking change, as there are updates to command line flag interactions. Changes included: - convert `--debug` bool flag -> `--log.level` string flag. this aligns with prom conventions. This project uses urfave/cli for application commands/flags rather than kingpin as other go packages do, so I couldn't use the promslog/flag pkg directly. Rather, I use our upstream promslog/kingpin configs in the configs we pass here for urfave/cli. We can decide to move off urfave/cli and/or adopt kingpin in the future. - removes `pkg/logging` in favor of using `prometheus/common/promslog`. A small utility function is included in main to simplify some boilerplate with logger initialization, since we instantiate in main in a few places. All functions have been converted to accept/use *slog.Loggers rather than the custom logger interface. - the removal of the custom logging pkg also removes the `IsDebugEnabled()` method, but this is fine because the logger is leveled; debug logs will not print unless `--log.level debug` is passed at the command line, there is no need to check if debug is enabled at log time in the vast majority of cases. - Updates tests as needed; updates nop loggers, test signatures that used `IsDebugEnabled()`, etc. - enable sloglint in golangci-lint, remove go-kit/log configs Signed-off-by: TJ Hoplock <[email protected]>
|
Thanks, we'll probably slowly convert over other things like |
|
Looking into the config verification failure in tests |
Code was totally valid, but it was setting the format twice. Intention is to set the level. 🤦 Prevents panics when using the logger due to level being nil: https://github.com/prometheus-community/yet-another-cloudwatch-exporter/actions/runs/12147649411/job/33881684148?pr=1586 Signed-off-by: TJ Hoplock <[email protected]>
Signed-off-by: TJ Hoplock <[email protected]>
|
Alright, I fixed up the verification test; panic was caused by a typo/wrong variable reference. Sorted out now 👍 |
|
Bump, lemme know if there's any other feedback 👍 |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I would call this a breaking change, as there are updates to command line flag interactions.
Changes included:
--debugbool flag ->--log.levelstring flag. this aligns with prom conventions. This project uses urfave/cli for application commands/flags rather than kingpin as other go packages do, so I couldn't use the promslog/flag pkg directly. Rather, I use our upstream promslog/kingpin configs in the configs we pass here for urfave/cli. We can decide to move off urfave/cli and/or adopt kingpin in the future.pkg/loggingin favor of usingprometheus/common/promslog. A small utility function is included in main to simplify some boilerplate with logger initialization, since we instantiate in main in a few places. All functions have been converted to accept/use *slog.Loggers rather than the custom logger interface.IsDebugEnabled()method, but this is fine because the logger is leveled; debug logs will not print unless--log.level debugis passed at the command line, there is no need to check if debug is enabled at log time in the vast majority of cases.IsDebugEnabled(), etc.