-
Notifications
You must be signed in to change notification settings - Fork 18
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add log_level configuration #121
base: main
Are you sure you want to change the base?
Conversation
This is to support the env variable OTEL_LOG_LEVEL. Fixes open-telemetry#120 Signed-off-by: Alex Boten <[email protected]>
Related: Until the spec defines an enum for log levels, opentelemetry-configuration can only represent a log level as a string, to be interpreted (differently) by each SDK implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This is the best we can do now, because the spec does not define an enum for log levels.
@@ -11,6 +11,9 @@ | |||
"disabled": { | |||
"type": ["boolean", "null"] | |||
}, | |||
"log_level": { | |||
"type": ["string", "null"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have an enum based on a subset of the SeverityNumber enum defined in the proto repo? https://github.com/open-telemetry/opentelemetry-proto/blob/4f69356d853029975649c3f38b06fc77d77975fc/opentelemetry/proto/logs/v1/logs.proto#L86-L110
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, though without a resolution on open-telemetry/opentelemetry-specification#2039, we may be jumping ahead of the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use the OTEL_LOG_LEVEL
property in opentelemetry-java
. For internal SDK logging, we use java util logging (or JUL for short), which comes with a default log level already, and has standard options for changing the log level on a per-library basis. For us, introducing an additional option would only muddy the log configuration story for users.
Therefore, I'm not a good judge of this property of its value since to me from the perspective of a java maintainer, it seems like a mistake. I won't approve, but also won't block progress.
I would expect this property to be used in configuring JUL in the java implementation, would that not be the case? Is there an alternative configuration property available to end users through the configuration file? |
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
I mean we'd have to have a discussion about it in the java SIG but my given that we've gone this long without supporting
No. Different tools for different things. The logging ecosystem in java is fractured and to complicated to explain here, but essentially users are used to the idea that they configure the logs from their libraries / frameworks in particular way. If we add support for this property, then there's the question of priority - should / can this property override the user's separate log config? A search for |
A quick search through the OpenTelemetry github org https://github.com/search?q=org%3Aopen-telemetry%20OTEL_LOG_LEVEL&type=code shows that it's used in a few different SIGs, so it probably makes sense to move forward with it. @brettmc can you review as it looks like PHP is one of the users of this env variable |
@codeboten sorry I'm late with the review, but the proposed usage would work for PHP's requirements for log level. 👍 |
This is to support the env variable OTEL_LOG_LEVEL.
Fixes #120