Incubating: Add minimum severity and trace-based logger configuration #7529
Conversation
…ers (#4612) Alternative to #4611 Some related prior discussions: - #4208 (comment) - #4207 (comment) Java POC @ open-telemetry/opentelemetry-java#7529 Declarative config for this is a bit nicer than in #4611, also supports applying different minimum severity levels to different loggers: ``` file_format: "1.0" logger_provider: logger_configurator/development: default_config: minimum_severity: WARN trace_based: true loggers: - name: "com.example.app.*" config: minimum_severity: INFO - name: "com.example.db.*" config: minimum_severity: ERROR ``` --------- Co-authored-by: Liudmila Molkova <[email protected]> Co-authored-by: Robert Pająk <[email protected]>
trask
force-pushed
the
minimum-severity-and-trace-based-log-configuration
branch
from
747bc91 to
f607dc9
Compare
trask
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7529 +/- ##
============================================
+ Coverage 90.16% 90.21% +0.04%
- Complexity 7229 7247 +18
============================================
Files 821 822 +1
Lines 21809 21828 +19
Branches 2136 2139 +3
============================================
+ Hits 19665 19693 +28
+ Misses 1472 1469 -3
+ Partials 672 666 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
trask
force-pushed
the
minimum-severity-and-trace-based-log-configuration
branch
3 times, most recently
from
d8f0544 to
c471c04
Compare
trask
force-pushed
the
minimum-severity-and-trace-based-log-configuration
branch
from
c471c04 to
6d538c3
Compare
| /** Returns a disabled {@link LoggerConfig}. */ | ||
| public static LoggerConfig disabled() { | ||
| return DISABLED_CONFIG; | ||
| } | ||
|
|
||
| /** Returns an enabled {@link LoggerConfig}. */ | ||
| public static LoggerConfig enabled() { | ||
| return DEFAULT_CONFIG; | ||
| } |
There was a problem hiding this comment.
do we want to keep these shortcuts? I'd lean towards removing them to avoid confusion that passing one of them at runtime only enables/disables (as opposed to resetting all the properties)
There was a problem hiding this comment.
The argument to keep them is for a consistent API across TracerConfig, MeterConfig, LoggerConfig.
A question to ask is whether there are sensible / intuitive default values for minimumSeverity, traceBased when you want it to call enabled() / disabled().
enabled(): I would expect this to be unopinionated on minimumSeverity, traceBased, and that's what we see in the defaults.disabled(): I would expect minimumSeverity, traceBased to not matter, since I expect all logs to be disabled.
So I don't see these methods as misleading. However, another argument to remove them is that they're probably not all that useful anymore. disabled() is still useful, but instead of enabled(), I'd expect more users to want a shortcut enable logs with severity greater than some threshold.
If we don't care about consistency with tracing and metrics, I think it would be most useful to have an API that accommodates the common scenarios users would want to express, since repreating .builder()...build() gets verbose. Something like:
enableAll();
enableAllSampled();
enableAboveSeverity(Severity);
enableSampledAboveSeverity(Severity);
disable();
Could also adjust them to return a Builder prepopulated with setEnabled(true) or setEnabled(false).
There was a problem hiding this comment.
enabled(): I would expect this to be unopinionated on minimumSeverity, traceBased, and that's what we see in the defaults.
won't calling updateLoggerConfig(LoggerConfig.enabled()) wipe out your existing minimumSeverity and traceBased configuration? that doesn't seem unopinionated to me 😄
sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLogRecordBuilder.java
Show resolved
Hide resolved
trask
force-pushed
the
minimum-severity-and-trace-based-log-configuration
branch
from
ff37f15 to
7131c10
Compare
trask
force-pushed
the
minimum-severity-and-trace-based-log-configuration
branch
from
7131c10 to
a88392e
Compare
sdk/logs/src/main/java/io/opentelemetry/sdk/logs/internal/LoggerConfig.java
Outdated
Show resolved
Hide resolved
sdk/logs/src/main/java/io/opentelemetry/sdk/logs/internal/LoggerConfig.java
Outdated
Show resolved
Hide resolved
| /** Returns a disabled {@link LoggerConfig}. */ | ||
| public static LoggerConfig disabled() { | ||
| return DISABLED_CONFIG; | ||
| } | ||
|
|
||
| /** Returns an enabled {@link LoggerConfig}. */ | ||
| public static LoggerConfig enabled() { | ||
| return DEFAULT_CONFIG; | ||
| } |
There was a problem hiding this comment.
The argument to keep them is for a consistent API across TracerConfig, MeterConfig, LoggerConfig.
A question to ask is whether there are sensible / intuitive default values for minimumSeverity, traceBased when you want it to call enabled() / disabled().
enabled(): I would expect this to be unopinionated on minimumSeverity, traceBased, and that's what we see in the defaults.disabled(): I would expect minimumSeverity, traceBased to not matter, since I expect all logs to be disabled.
So I don't see these methods as misleading. However, another argument to remove them is that they're probably not all that useful anymore. disabled() is still useful, but instead of enabled(), I'd expect more users to want a shortcut enable logs with severity greater than some threshold.
If we don't care about consistency with tracing and metrics, I think it would be most useful to have an API that accommodates the common scenarios users would want to express, since repreating .builder()...build() gets verbose. Something like:
enableAll();
enableAllSampled();
enableAboveSeverity(Severity);
enableSampledAboveSeverity(Severity);
disable();
Could also adjust them to return a Builder prepopulated with setEnabled(true) or setEnabled(false).
…d-trace-based-log-configuration
2 participants
Incubating implementation for open-telemetry/opentelemetry-specification#4612