promslog: Make AllowedLevel concurrency safe. #754
Conversation
Needed for prometheus/prometheus#10352 Also I renamed AllowedLevel and AllowedFormat to Level and Format. Default level (and String()) is also now 'info' not empty. It's a breaking change, but I suspect nobody was using those constructs directly, WDYT? Signed-off-by: bwplotka <bwplotka@gmail.com>
| err := yaml.Unmarshal([]byte(``), l) | ||
| if err != nil { | ||
| t.Error(err) | ||
| } | ||
| if l.s != "" { | ||
| t.Errorf("expected empty level, got %s", l.s) | ||
| if got := l.String(); got != "info" { |
There was a problem hiding this comment.
I changed the behaviour purposefully. This simplifies (and optimizes) the level code.
I don't know exactly how Unmarshal and String was envisioned to be used in the YAML context. Does this blocks anyone?
| type AllowedFormat struct { | ||
| // Format controls a logging output format. | ||
| // Not concurrency-safe. | ||
| type Format struct { |
There was a problem hiding this comment.
As mentioned in the description - this is a breaking change, let me know if this is ok.
There was a problem hiding this comment.
Yes, OK this breaking change.
Any chance you would keep the old struct around and add a Deprecated comment so downstream Go users get a linter warning?
There was a problem hiding this comment.
Or, maybe it's better to just drop the struct so it's a compile error.
There was a problem hiding this comment.
For deprecated flow we would need to maintain a behaviour which means promslog.Config using old thing, we can but until we are v1 and this is not too painful we can break.
| // Level controls a logging level, with an info default. | ||
| // It wraps slog.LevelVar with string-based level control. | ||
| // Level is safe to be used concurrently. | ||
| type Level struct { |
There was a problem hiding this comment.
As mentioned in the description - this (rename) is a breaking change, let me know if this is ok.
Signed-off-by: bwplotka <bwplotka@gmail.com>
There are at least a few that I know of: promtheus json file logger: prom api testing blackbox exporter custom probe logging (handler.go and handler_test.go) Updates shouldn't be difficult, but a few will be needed after the breaking change |
See prometheus/common#754 Signed-off-by: Richard Kosegi <richard.kosegi@gmail.com>
See prometheus/common#754 Signed-off-by: Richard Kosegi <richard.kosegi@gmail.com>
|
Hi there. Thanks a stack for your work on this package!
Our code apparently used one of the symbols, but we have been able to resolve that speed bump quickly. NB: This is just FYI for people who may run into the same problem. |
Immediate fix for prometheus#1377. This fix will need to be refactored a bit when we upgrade prometheus/common to v0.63.0, since prometheus/common#754 contains a breaking change to promslog's level code. Explanation: When I ported the blackbox exporter to use slog in prometheus#1311, I removed the `none` option from the log prober flag, as it was non-standard to other prometheus projects, not a valid value in the original promlog package, and slog has no inherent corresponding concept. The log prober flag value is used to create the scrape logger (which is specifically used for scrape-related logging, not overall application logging). I also included some code that ensured a level config would be created for use with the scrape logger. In combination, this causes the blackbox exporter to always output scrape logs, which can be noisy. If we instead default the flag to an empty string and do some intelligent handling of an nil log prober value within the scrapeLogger's implementation of the slog.Handler interface, then we can restore the previous behavior. TL;DR: Leave `--log.prober` flag unset and it won't do scrape-level logging Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
Immediate fix for prometheus#1377. This fix will need to be refactored a bit when we upgrade prometheus/common to v0.63.0, since prometheus/common#754 contains a breaking change to promslog's level code. Explanation: When I ported the blackbox exporter to use slog in prometheus#1311, I removed the `none` option from the log prober flag, as it was non-standard to other prometheus projects, not a valid value in the original promlog package, and slog has no inherent corresponding concept. The log prober flag value is used to create the scrape logger (which is specifically used for scrape-related logging, not overall application logging). I also included some code that ensured a level config would be created for use with the scrape logger. In combination, this causes the blackbox exporter to always output scrape logs, which can be noisy. If we instead default the flag to an empty string and do some intelligent handling of an nil log prober value within the scrapeLogger's implementation of the slog.Handler interface, then we can restore the previous behavior. TL;DR: Leave `--log.prober` flag unset and it won't do scrape-level logging Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
…3.0 (#12656)" (#12673) #### Description Contrib tests are currently failing on main with the following build error: ``` Error: /home/runner/go/pkg/mod/github.com/prometheus/prometheus@v0.300.1/util/logging/file.go:42:23: undefined: promslog.AllowedFormat ``` I believe this is because of the update in #12656: there is [a breaking change in `github.com/prometheus/common` 0.63.0](prometheus/common#754) that is incompatible with the version of `github.com/prometheus` used in some contrib modules. While the latter package [has been updated](prometheus/prometheus#16210) on main, the updated version hasn't been released yet, so I this the only solution is to revert this PR until then.
Needed for prometheus/prometheus#10352
Also I renamed AllowedLevel and AllowedFormat to Level and Format. Default level (and String()) is also now 'info' not empty.
It's a breaking change, but I suspect nobody was using those constructs directly, WDYT?
cc @tjhop