Skip to content
This repository was archived by the owner on Nov 10, 2022. It is now read-only.

No reasonable way to use OTEL_LOG_LEVEL #102

Closed
jtmalinowski opened this issue Jul 8, 2021 · 13 comments
Closed

No reasonable way to use OTEL_LOG_LEVEL #102

jtmalinowski opened this issue Jul 8, 2021 · 13 comments

Comments

@jtmalinowski
Copy link

jtmalinowski commented Jul 8, 2021

Problem 1: just setting the logger uses a default level instead of the one from env configuration (https://github.com/open-telemetry/opentelemetry-js-api/blob/main/src/api/diag.ts#L75). Which means when setting a logger you need to fetch the ENV variable by yourself

diag.setLogger(new DiagConsoleLogger(), getEnv().OTEL_LOG_LEVEL);

which sort of defeats the point of the ENV variable?

Problem 2: tangentially, after setLogLevel was removed, there's no way to set the level and the logger independently (#9), which will make integration with Node ENV configuration tricky.

I'd propose:

  • reviving setLogLevel
  • change setLogger so that second argument, unless provided, doesn't change the log level
  • allow setLogLevel and setLogger to be called in any order

Also, I'm happy to hear your thoughts, and send a PR when a resolution is decided.

ping @dyladan

@longility
Copy link

I was surprised when I set OTEL_LOG_LEVEL that it did nothing. I agree that is misleading.

@dyladan
Copy link
Member

dyladan commented Aug 27, 2021

@MSNev what do you think? I agree that our current logging situation is not ideal and would like to make it easier to enable debugging.

@MSNev
Copy link
Contributor

MSNev commented Aug 27, 2021

Agree, from (vague) memory this is why I had it before.

So

reviving setLogLevel

Yes

change setLogger so that second argument, unless provided, doesn't change the log level

Yes

allow setLogLevel and setLogger to be called in any order

Yes

@dyladan
Copy link
Member

dyladan commented Aug 27, 2021

@MSNev or @longility would you like to make a PR?

@longility
Copy link

If I have time next week, I'll let you know. @MSNev feel free to take it you got time.

@MSNev
Copy link
Contributor

MSNev commented Aug 27, 2021

I don't currently have cycles and I'm going to be OOF again in a few weeks, so go for it.

@longility
Copy link

If the logger is not set, should we be defaulting to a noop logger or DiagConsoleLogger?

@longility
Copy link

Another option could be if env var OTEL_LOG_LEVEL is set than we default to DiagConsoleLogger. If not set, then default to noop.

@dyladan
Copy link
Member

dyladan commented Aug 30, 2021

Another option could be if env var OTEL_LOG_LEVEL is set than we default to DiagConsoleLogger. If not set, then default to noop.

Seems a reasonable compromise

@longility
Copy link

longility commented Sep 18, 2021

Acceptance criteria for whoever ends up tackling.

I may have time next week.

  • If OTEL_LOG_LEVEL is defined, then default to DiagConsoleLogger.
  • Logger set through setLogger should overwrite the default logger.
  • If OTEL_LOG_LEVEL is not defined, then default to NoopLogger.
  • setLogger to maintain existing log level and not overwrite with parameter default. (i.e. setLogger without log level provides should not overwrite the defined OTEL_LOG_LEVEL)
  • Revive setLogLevel and allow setLogger and setLogLevel to be called and set in any order. Natural set order.

@longility
Copy link

If OTEL_LOG_LEVEL is defined, then default to DiagConsoleLogger.

I'm actually trying to figure out where this should go as getEnv() is in @opentelemetry/core and there isn't a pattern that I know of that is gets executed "automatically" without end user trigger from configuration.

Basically, without otel configuration and only env var OTEL_LOG_LEVEL where do we call something like if(getEnv().OTEL_LOG_LEVEL) diag.setLogger(new DiagConsoleLogger(), getEnv().OTEL_LOG_LEVEL);?

Personally, this is the only acceptance criteria that I desire among the others and it may be the 80% case. I think the other acceptance criterias are 20% case.

@jonaskello
Copy link

jonaskello commented Nov 19, 2021

I tried for the longest time to make OTEL_LOG_LEVEL work and finally found this issue. Added the code below and now it works:

  api.diag.setLogger(new DiagConsoleLogger(), getEnv().OTEL_LOG_LEVEL);

@dyladan
Copy link
Member

dyladan commented Oct 10, 2022

Moved this to the main repo where the API now resides open-telemetry/opentelemetry-js#3312

@dyladan dyladan closed this as completed Oct 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants