Skip to content
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

Clarify values for OTEL_LOG_LEVEL #920

Open
ffe4 opened this issue Sep 3, 2020 · 8 comments
Open

Clarify values for OTEL_LOG_LEVEL #920

ffe4 opened this issue Sep 3, 2020 · 8 comments
Labels
area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:miscellaneous For issues that don't match any other spec label

Comments

@ffe4
Copy link
Contributor

ffe4 commented Sep 3, 2020

The spec defines an OTEL_LOG_LEVEL environment variable for general SDK configuration, but it is unclear what values should be allowed. Since this variable does not have a language prefix, there should probably be a unified set of log levels specified.

We could link to the Log Data Model, which already specifies log levels.

@ffe4 ffe4 added the spec:miscellaneous For issues that don't match any other spec label label Sep 3, 2020
@arminru arminru added the area:sdk Related to the SDK label Sep 4, 2020
@andrewhsu andrewhsu added priority:p3 Lowest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 8, 2020
@arminru
Copy link
Member

arminru commented Sep 8, 2020

From the SIG spec meeting: We are not sure if we can properly align the log level across all frameworks that may be plugged into each SDK and should reconsider if OTEL_LOG_LEVEL can be implemented properly at all. We should consider removing it from GA and replace it with a simple "debug flag" for now, that would allow fundamental troubleshooting, and consider coming up with a proper, more fine-grained solution later on.

Apart from that, the name OTEL_LOG_LEVEL might be confusing and could collide with environment variables that the Logging SIG will need in future, so we should at least rename it to something like OTEL_SDK_DIAGNOSTIC_LOG_LEVEL to disambiguate it, if we decide to keep it.

@jkwatson
Copy link
Contributor

jkwatson commented Sep 8, 2020

Even with adding a "debug" flag, for Java, I would expect to just set my logging implementation to log io.opentelemetry.sdk at DEBUG, rather than doing this through a separate env var. If we add a separate "debug" setting, that would mean additionally surrounding every logging statement in the SDK with an additional check. Do we want this? Or do we want this to be a language-specific consideration?

@iNikem
Copy link
Contributor

iNikem commented Sep 8, 2020

If we add a separate "debug" setting, that would mean additionally surrounding every logging statement in the SDK with an additional check

Not necessarily. We can check for the presence of debug tag and then programmatically set my logging implementation to log io.opentelemetry.sdk at DEBUG

@jkwatson
Copy link
Contributor

jkwatson commented Sep 8, 2020

@iNikem but...we don't know "my logging implementation" in the SDK, do we? This implementation could be log4j, slf4j-simple, logback, etc.

@iNikem
Copy link
Contributor

iNikem commented Sep 8, 2020

We are talking about SDK logging, write? We can configure java.util.logging level, cannot we? Or am I mistaken?

@jkwatson
Copy link
Contributor

jkwatson commented Sep 8, 2020

We are talking about SDK logging, write? We can configure java.util.logging level, cannot we? Or am I mistaken?

How does that work if the underlying logging is all being redirected to another logging implementation? I'm actually not sure how that ends up working, TBH.

@arminru arminru added release:after-ga Not required before GA release, and not going to work on before GA and removed priority:p3 Lowest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 25, 2020
@ffe4
Copy link
Contributor Author

ffe4 commented Nov 14, 2020

OTEL_LOG_LEVEL is still on the spec compliance matrix, and some languages have already implemented it. I think it would be a good idea to solve this issue before GA, especially if we want to reserve the name for the Logging SIG.

Implementing a DEBUG flag sounds like a good compromise. SIGs can still choose to provide a language specific variable for logging levels, such as OTEL_{LANGUAGE}_SDK_LOG_LEVEL, if they feel the need to.

@MSNev
Copy link
Contributor

MSNev commented Jun 1, 2021

This does need to be clarified and a while back @tedsuo bought up that we should define the API/SDK Diagnostic logging discussion to happen after the convenience API and I was going to help drive this.

From a JS API perspective we have already defined and using a Diagnostic API and we have defined the following levels which are used for the SDK OTEL_LOG_LEVEL, note that we only support this environment variable being defined as a "String" so that we are free to fiddle with the actual enumeration values within the API/SDK

From my perspective we should NOT link this with the Logging specification as these are different concepts, but the renaming to something more specific would be nice to avoid overloading perhaps OTEL_DIAG_LOG_LEVEL and as per the JS implementation have it defined with string mapping with the minimum set of levels, this would provide a defined specification level values while still allowing SDK's "fill in" any SDK/language specific blanks or optionally support numeric values without specifically invalidating the specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:miscellaneous For issues that don't match any other spec label
Projects
None yet
Development

No branches or pull requests

6 participants