Allow LoggerProvider to be specified in Instrumentations

[hectorhdzg]

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Allow LoggerProvider to be set for Instrumentations, so auto generation code for logs can use it, Bunyan, Winston, Pino, etc.

Fixes # (issue)

Short description of the changes

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Unit tests

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[codecov]

Codecov Report

Merging #4314 (aab138b) into main (0229434) will increase coverage by 0.03%.
The diff coverage is 83.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4314      +/-   ##
==========================================
+ Coverage   92.31%   92.35%   +0.03%     
==========================================
  Files         108      328     +220     
  Lines        2213     9455    +7242     
  Branches      428     2011    +1583     
==========================================
+ Hits         2043     8732    +6689     
- Misses        170      723     +553     

Files	Coverage Δ
...es/opentelemetry-instrumentation/src/autoLoader.ts	100.00% <100.00%> (ø)
...entelemetry-instrumentation/src/autoLoaderUtils.ts	92.85% <100.00%> (ø)
...entelemetry-instrumentation/src/instrumentation.ts	77.77% <66.66%> (ø)

... and 228 files with indirect coverage changes

[hectorhdzg]

This functionality was discussed before in open-telemetry/opentelemetry-js-contrib#1713 (comment), thanks @trentm for letting me know about that, we can have different classes for Trace Instrumentations and Log Instrumenations, so we can have the definition of it and shared functionality in the same place avoiding different behaviors and duplication of code, @blumamir @pichlermarc thoughts?

[dyladan]

Looks good. Can you please add tests for this functionality?

[trentm]

This functionality was discussed before in open-telemetry/opentelemetry-js-contrib#1713 (comment)

Note that in the earlier discussion, we had been leaning towards not adding a this._logger to every instrumentation.

Other related comments from that issue:

From open-telemetry/opentelemetry-js-contrib#1713 (comment)

well then it would be good to have a specific Log Instrumentation then or similar

open-telemetry/opentelemetry-js-contrib#1713 (comment)

yeah having a LoggingInstrumentationConfig and a LoggingInstrumentation in @opentelemetry/instrumentation starts to make sense to me, at least for now we want to have, Winston, Bunyan, Pino and Console to also generate log telemetry, so this is something that several package would benefit from.

[pichlermarc]

I think holding off on this feature for now is a better idea.

The Logs API is not stable yet, and while @opentelemetry/instrumentation is also not stable I think we should start moving towards this. Also I think right now a very small percentage of instrumentations would make use of the logger for now.

I'm also not sure about the idea of having a separate class for Logging instrumentation, as that could make it difficult (it may encourage double wrapping libraries, for traces and for logs, and then there's a large potential of bugs where another instrumentation interferes with another one)

Just some thoughts, happy to be convinced otherwise though 🙂

[hectorhdzg]

@pichlermarc related to logs not being stable, I believe we should be pushing to stabilize both Log API and Instrumentation, Logs is quite important as well, we are working on adding more instrumentations including winston, bunyan and console to use the logger, so this will start to grow very soon, I'm more in favor of having Logger available for all instrumentations, would be similar to Meter, some instrumentations will use it and some other don't, what is preventing us to stabilize Logs API?

[trentm]

I'm more in favor of having Logger available for all instrumentations

But shouldn't the only instrumentations that would use a this._logger property be those that are instrumenting logging frameworks? For all other instrumentations this._logger should be unused.

[pichlermarc]

I'm more in favor of having Logger available for all instrumentations

But shouldn't the only instrumentations that would use a this._logger property be those that are instrumenting logging frameworks? For all other instrumentations this._logger should be unused.

I agree, to my understanding this is the main idea of the Logs bridge. With events, I'd expect this to be a different story.

what is preventing us to stabilize Logs API?

The API spec is stable so it's mainly a question of available bandwidth in the SIG. Anyone can pick up and drive the topic - in fact it would be highly appreciated. There has not been a lot of work after the initial prototyping so the first step would be to re-review the API based on spec compliance, then fix any issues - the same with the logs SDK. Then we would request a review from the Technical Committee, after that we integrate the API into the @opentelemetry/api package and move the logs sdk to the stable packages, at which point they will be released as stable.

[hectorhdzg]

@pichlermarc great news, I would be more than happy to start the discussion of stabilizing Logs API, @trentm I understand logger would not be used by all instrumentations but what is the issue of logger being there?, instrumentations using the logger would be deviating from the common way and that is why I created this PR, without this the instrumentations will always use global loggerProvider and maybe that is ok but why having the options to specify providers in autoLoader in the first place then

[trentm]

without this the instrumentations will always use global loggerProvider and maybe that is ok but why having the options to specify providers in autoLoader in the first place then

Ah, that is a good argument for this.

https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation/README.md#selection-of-the-used-tracerprovidermeterprovider mentions "There might be usecase" for specifying providers other than the global ones for instrumentations. I haven't used this myself, so I don't know what use cases were in mind for that. I see a fair amount of .setTracerProvider(...) usage in tests in the contrib repo, so that seems to be at least one useful case.

I'm now okay with this change.

[trentm]

^^ sorry for that self-assigning and unassigning.

[pichlermarc]

I am now also in favor (we discussed this in a SIG meeting some time ago) and we came to the conclusion that adding it would be beneficial.

Looks good, thanks!

[trentm]

LGTM, but nit about the changelog item now needing to move up.

[hectorhdzg]

@pichlermarc @dyladan looks like this PR is ready to merge, let me know if there is any additional feedback