feat(api-logs): Add delegating no-op logger provider #4861
Conversation
hectorhdzg
changed the title
|
Hey Hector, I see that you marked this as "Breaking change", would you mind providing more info on that? |
| export * from './ProxyLogger'; | ||
| export * from './ProxyLoggerProvider'; |
There was a problem hiding this comment.
after #4880 landed, this should be changed to named exports
| @@ -48,6 +51,7 @@ export class LogsAPI { | |||
| provider, | |||
| NOOP_LOGGER_PROVIDER | |||
| ); | |||
| this._proxyLoggerProvider.setDelegate(provider); | |||
|
|
|||
| return provider; | |||
There was a problem hiding this comment.
Aside: I was comparing this setGlobal... to the equivalent for the TracerProvider and noticed that the internal/global-utils.ts and global registration handling is very different in api-logs from what it is in the "api" package. Apparently the code here was copied from api-metrics when that was a separate package ... but before the api-metrics package's global-registration handling was changed to #3357
When the api-logs package is merged into the "api" package, it will have to adapt to this new global-utils handling. That could be done before or after this PR. Until then, the global registration handling is subtley different between them.
(api-events will want to change as well)
There was a problem hiding this comment.
Yeah I was planning to take care of that with api-logs migration to api package PR but that one got complicated, I will take care of using same global utils in different PR.
| @@ -7,6 +7,7 @@ All notable changes to experimental packages in this project will be documented | |||
|
|
|||
| ### :boom: Breaking Change | |||
|
|
|||
| * feat(api-logs): Add delegating no-op logger provider [#4861](https://github.com/open-telemetry/opentelemetry-js/pull/4861) @hectorhdzg | |||
There was a problem hiding this comment.
[marylia]
Hey Hector, I see that you marked this as "Breaking change", would you mind providing more info on that?
[Hector]
this is a breaking change because the behavior of logs.getLoggerProvider will return a ProxyLoggerProvider instead of the actual registered logger provider, same as current behavior for tracerProvider
Is that really a breaking change? logs.getLoggerProvider returns an object of type interface LoggerProvider both before and after, so I wonder (and hope) this could be considered not a breaking change.
A breaking change isn't so big a deal for this package, because it is still 0.x. However, a breaking change for the delegating MeterProvider in #4858, which is part of the "api" package, would be a very big deal: there is a VERY strong disincentive to have a major version bump for the "api" package.
There was a problem hiding this comment.
@hectorhdzg We discussed in the SIG today (I brought it up). We agreed that we do not need to consider this a breaking change, because the exported types for these APIs are interfaces.
There was a problem hiding this comment.
Thanks @trentm would update changelog then
|
@trentm any other feedback in this PR?, I believe this one is ready to be merged |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Which problem is this PR solving?
Fixes #4399
Adding ProxyLoggerProvider and ProxyLogger
Type of change
Checklist: