refactor(logger): provide getLogger helper and remove NoopLogger class implementation [#1754]

[MSNev]

Which problem is this PR solving?

• Provides solution for [API Suggestion] Provide getLogger helper and remove NoopLogger class implementation #1754
• Supports minification by
  • Reducing the amount of boiler plate code that has to be written when the code needs to ensure that it has a non-null/undefined logger instance
  • Removing unnecessary class definition (NoopLogger)

Short description of the changes

• Removes the NoopLogger from the api
• Adds a helper getLogger(logger?: Logger | null) to core
  • Returns either the passed logger (if valid/defined) or a Noop implementation

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Nev (66e1b86)

[codecov]

Codecov Report

Merging #1844 (e4de0f5) into master (1a3f3f8) will decrease coverage by 0.35%.
The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master    #1844      +/-   ##
==========================================
- Coverage   92.62%   92.26%   -0.36%     
==========================================
  Files         174      159      -15     
  Lines        6045     5199     -846     
  Branches     1286     1107     -179     
==========================================
- Hits         5599     4797     -802     
+ Misses        446      402      -44     

Impacted Files	Coverage Δ
...entelemetry-instrumentation/src/instrumentation.ts	65.21% <60.00%> (ø)
packages/opentelemetry-api/src/trace/getLogger.ts	100.00% <100.00%> (ø)
...elemetry-core/src/context/propagation/composite.ts	100.00% <100.00%> (ø)
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts	87.65% <100.00%> (ø)
...etry-exporter-prometheus/src/PrometheusExporter.ts	92.30% <100.00%> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	100.00% <100.00%> (ø)
...es/opentelemetry-instrumentation/src/autoLoader.ts	100.00% <100.00%> (ø)
...ackages/opentelemetry-metrics/src/BatchObserver.ts	100.00% <100.00%> (ø)
packages/opentelemetry-metrics/src/Metric.ts	92.10% <100.00%> (ø)
...ry-resources/src/platform/node/detect-resources.ts	20.83% <100.00%> (ø)
... and 16 more

[obecny]

I don't think it is a good idea to force people to use sdk everywhere to avoid using NoopLogger in favor of getLogger from core. Even in description of #1754 you mentioned api function and here you added a core function. This will force all packages depends on core which is wrong and we just recently removed the core dependency of NoopLogger because of that.
If I remember correctly main reason was to try to minify it better and to have some simple draft to see the difference. How many bytes do we gain by this refactoring as for me it looks like now we will have maybe even more if you move the getLogger to api from core. Can you make such comparison before and after ?
Also the getLogger function is a bit overkilled it does too many things already. It should only fallback to Noop implementation, ConsoleLogger should not be a part of it.

[MSNev]

I don't think it is a good idea to force people to use sdk everywhere to avoid using NoopLogger in favor of getLogger from core.

My understanding is that the API is supposed to be interface only and therefore should no have any implementations...
And I believe that NoopLogger is not part of the OT Api specification... Which is also part of the reason for moving it (back) to core, the other was to incorporate ConsoleLogger support.

I can move the getLogger() (and remove ConsoleLogger fallback) to API, but it seems to be the wrong direction (IMHO anyway).

This will force all packages depends on core which is wrong and we just recently removed the core dependency of NoopLogger because of that.

All packages (except instrumentation) already have a dependency on core, and based on the above this seems correct. Technically, if a package doesn't want to have external links (NoopLogger for instrumentation) then it probably should be handling an undefined logger rather than requiring a NoopLogger() implementation etc. (This seems like a larger discussion)

If I remember correctly main reason was to try to minify it better and to have some simple draft to see the difference. How many bytes do we gain by this refactoring as for me it looks like now we will have maybe even more if you move the getLogger to api from core. Can you make such comparison before and after ?

Sure, but there doesn't currently to be any minification step as part of the build -- this might take a while before I can get enough cycles to do this as well.

Also the getLogger function is a bit overkilled it does too many things already. It should only fallback to Noop implementation, ConsoleLogger should not be a part of it.

I partially agree, the only reason I added was because of the existing usage -- a better approach I think would be if have a "default" logger as the fallback, which this provides a possible platform to "add" a setDefaultLogger() implementation.

[Flarna]

The API contains also noop implementations for e.g Tracer.
One target of OTel is that whole SDK can be replaced by a different one and therefore a dependency to core is not intended.

There were quite some PRs in the past to move parts from core to api/context/instrumentation.

[MSNev]

Removing ConsoleLogger changes (added issue #1847) and moving getLogger to api package

[MSNev]

As discussed in the SIG, I'm going to abandon this PR and change it into providing a "diag" constant at the api level and then change the code to use this value

• Keeping (and deprecating) the existing Logger interface, NoopLogger and ConsoleLogger classes so we don't immediately break the contrib repro and we will have a follow up PR that eventually removes them