fix: stop scrape logger spam, refactor scrape logger design#1381
Conversation
bb17f7e to
7a291b8
Compare
Based on discussion in prometheus#1377, slack, etc, it seems unclear what the intended functionality/behavior of the probe scrape logger was supposed to be. This is an attempt to align behavior with current user expectations. The improved defaults and updated configs here should address the log spam as well. User facing changes: - the `--log.prober` flag now sets the _level at which prober scrape logs will be emitted at_. - the `--log.prober` flag now defaults to `debug` level. Because the default log level is info, this means that in order to see probe scrape logs, a user must either: - increase the level of the logger via `--log.level` to be >= that of `--log.prober` (ie, set both to `debug`) - decrease the level of the probe logger via `--log.prober` to be <= that of `--log.level` (ie, set both to `info`) Note: This approach (emitting all logs from the probe logger at the level specified) involves hijacking calls to the logger and overriding the level. This comes with the very obvious side effect that the developers working on code using the probe scrape logger no longer have control over the control over the level of the log that gets emitted. Regardless of whether they use `l.Info()` or `l.Debug()`, it'll still get logged at the configured log prober level. This approach likely would not be compatible with some of the other discussions around how probe scrape logs work, like prometheus#1401. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
7a291b8 to
7df3031
Compare
|
@SuperQ @electron0zero I believe this is ready for review if you find yourselves with some spare time 🙃 |
Address PR feedback Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
|
thank you for your contribution 👏🏼 |
|
@tjhop I feel like I am misunderstanding the In the docs, you provide example output for different combinations of My understanding of log levels is that "increasing" the log level (e.g. going from INFO to DEBUG in this case) should only ever "add" log output, meaning that the log lines emitted at level DEBUG should also include all lines that would be emitted at level INFO (and the log level of those messages would still show up as "INFO"). Is my understanding of log levels incorrect, or am I maybe misunderstanding what we are trying to achieve here? (I have read the discussion in #1377 but am still unsure...) |
|
related question: Is it (still?) possible to configure log levels in the config file? |
|
Now when I run the curl command: We did something strange. |
|
@paketb0te no worries, this is a non-standard feature anyway, and the way it "merges" log streams isn't super intuitive (which is why it's taking us so long to work out exactly how this should work). Here's an analogy/mental model that I find helpful when trying to understand what the scrape probe logger is doing now: Let's imagine logging is like a railroad system, with an individual railway for each of the log levels: info, warn, error, debug. These are the only railways that are routeable to a destination, and the verbosity levels in the Here's a decent text diagram I was able to get of the logical flow that I was able to get gemini to make for me: Does that help to clarify things at all? |
|
@tentakle yea, this whole feature is a tad strange 🙃 Good catch, though! I was able to replicate, and I'm working on a fix for use with the |
Fixes issue reported here: prometheus#1381 (comment) In aligning behavior for how the `--log.prober` flag is currently expected to work, I didn't test it's effect on logs when used with the `debug` URL parameter, as is often done for testing. This changes the handler to always write to the internal buffer logger (which is what is used to store the logs for displaying with the `debug` url param), and it only writes through to the real logger if it's enabled for the level the prober is set to. This ensures that logs are always available with the `debug` param, while still correctly filtering levels for console output at stderr. As a consequence of thinking about how to make this work, I also realized it wasn't necessary to manually iterate record attrs or maintain our own attr list when hijacking the log call -- the level can be overridden directly from the record, we just need to grab a copy via Clone() first. So should be more efficient, as well. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
Fixes issue reported here: #1381 (comment) In aligning behavior for how the `--log.prober` flag is currently expected to work, I didn't test it's effect on logs when used with the `debug` URL parameter, as is often done for testing. This changes the handler to always write to the internal buffer logger (which is what is used to store the logs for displaying with the `debug` url param), and it only writes through to the real logger if it's enabled for the level the prober is set to. This ensures that logs are always available with the `debug` param, while still correctly filtering levels for console output at stderr. As a consequence of thinking about how to make this work, I also realized it wasn't necessary to manually iterate record attrs or maintain our own attr list when hijacking the log call -- the level can be overridden directly from the record, we just need to grab a copy via Clone() first. So should be more efficient, as well. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
Based on discussion in #1377, slack, etc, it seems unclear what the
intended functionality/behavior of the probe scrape logger was supposed to be.
This is an attempt to align behavior with current user expectations. The
improved defaults and updated configs here should address the log spam
as well.
User facing changes:
--log.proberflag now sets the level at which prober scrapelogs will be emitted at.
--log.proberflag now defaults todebuglevel. Because thedefault log level is info, this means that in order to see probe
scrape logs, a user must either:
--log.levelto be >= thatof
--log.prober(ie, set both todebug)--log.proberto be <=that of
--log.level(ie, set both toinfo)Note:
This approach (emitting all logs from the probe logger at the level
specified) involves hijacking calls to the logger and overriding the
level. This comes with the very obvious side effect that the developers
working on code using the probe scrape logger no longer have control
over the control over the level of the log that gets emitted. Regardless
of whether they use
l.Info()orl.Debug(), it'll still get logged atthe configured log prober level. This approach likely would not be
compatible with some of the other discussions around how probe scrape
logs work, like #1401.
Signed-off-by: TJ Hoplock t.hoplock@gmail.com