refactor(sdk-trace-base, sdk-node): add TracerProvider class and use BasicTracerProvider in Node SDK#6640
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6640 +/- ##
==========================================
+ Coverage 94.85% 94.87% +0.01%
==========================================
Files 377 378 +1
Lines 12751 12789 +38
Branches 2887 2903 +16
==========================================
+ Hits 12095 12133 +38
Misses 656 656
🚀 New features to boost your workflow:
|
@opentelemetry/sdk-trace-nodeTracerProvider class and resolve traces config from env in Node SDK
trentm
left a comment
There was a problem hiding this comment.
Unsolicited feedback. I know this is still in draft. :)
| @@ -24,105 +23,30 @@ export enum ForceFlushState { | |||
| /** | |||
| * This class represents a basic tracer provider which platform libraries can extend | |||
There was a problem hiding this comment.
The PR desc suggests you were going to mark this class as deprecated.
There was a problem hiding this comment.
right, I'll add the deprecation mark
There was a problem hiding this comment.
Thanks. However if we don't have a replacement yet, should we not deprecate it yet?
My (minor) concern is that users of BasicTracerProvider will get a deprecation marker in their editor/lint output (maybe), but without an alternative to use yet.
Or I suppose you could point to the TracerProvider being adding in this PR, but warning users that it no longer handles reading env vars. Hrm. That's not yet satisfying.
Eventually perhaps there is a createTracerProviderFromEnv() or similar function export from the sdk-node package.
There was a problem hiding this comment.
My (minor) concern is that users of BasicTracerProvider will get a deprecation marker in their editor/lint output (maybe), but without an alternative to use yet.
I think it's a valid concern. What we can do is:
- remove the deprecation
- move on with SDK 3.0
- once published add the deprecation notice and do a 2.X release
There was a problem hiding this comment.
I suppose we won't consider dropping BasicTracerProvider in 3.0, because it is coming so soon?
If so, then yes, deprecating it some time after SDK 3.0 is released sounds good.
| /** | ||
| * TracerProviderConfig provides an interface for configuring a Tracer Provider. | ||
| */ | ||
| export type TracerProviderConfig = TracerConfig; |
There was a problem hiding this comment.
I'll rename it to TracerProviderOptions then :)
There was a problem hiding this comment.
TracerProviderConfig is fine. The issue is that TracerConfig is currently being used (as config for a TracerProvider) in the existing released packages for something different than what the spec now uses TracerConfig for (as config for a Tracer).
There was a problem hiding this comment.
The issue is that TracerConfig is currently being used (as config for a TracerProvider)...
I thought on changing the name but I think it can wait to the new major.
TracerProvider class and resolve traces config from env in Node SDKTracerProvider class and use BasicTracerProvider in Node SDK
trentm
left a comment
There was a problem hiding this comment.
Just a partial review. I have to run and don't want to forget to submit this so far.
There was a problem hiding this comment.
- Perhaps also get the sdk-trace-node usages in the instr-http and instr-grpc README.md files?
- Can the sdk-trace-node devDep be removed from those package.json files?
|
|
||
| * test(otlp-transformer): add metrics transform benchmark [#6628](https://github.com/open-telemetry/opentelemetry-js/pull/6628) @pichlermarc | ||
| * refactor(opentelemetry-exporter-prometheus): do not call enforcePrometheusNamingConvention() multiple times per metric [#6636](https://github.com/open-telemetry/opentelemetry-js/pull/6636) @cjihrig | ||
| * refactor(sdk-node): replace usage of `NodeTracerProvider` for `BasicTracerProvider` in SDK start [#6640](https://github.com/open-telemetry/opentelemetry-js/pull/6640) @david-luna |
There was a problem hiding this comment.
Reminder to move this up to the top Unreleased section when close to merging.
| @@ -24,105 +23,30 @@ export enum ForceFlushState { | |||
| /** | |||
| * This class represents a basic tracer provider which platform libraries can extend | |||
There was a problem hiding this comment.
Thanks. However if we don't have a replacement yet, should we not deprecate it yet?
My (minor) concern is that users of BasicTracerProvider will get a deprecation marker in their editor/lint output (maybe), but without an alternative to use yet.
Or I suppose you could point to the TracerProvider being adding in this PR, but warning users that it no longer handles reading env vars. Hrm. That's not yet satisfying.
Eventually perhaps there is a createTracerProviderFromEnv() or similar function export from the sdk-node package.
| import { reconfigureLimits } from './utility'; | ||
| import { TracerProvider } from './TracerProvider'; | ||
|
|
||
| export enum ForceFlushState { |
There was a problem hiding this comment.
Guessing this can be dropped. I didn't check if all uses in this file were removed, and moved to TracerProvider.ts
| import { AlwaysOnSampler } from './sampler/AlwaysOnSampler'; | ||
| import { RandomIdGenerator } from './platform'; | ||
|
|
||
| export enum ForceFlushState { |
There was a problem hiding this comment.
Does this need to be exported? This could totally be addressed in a separate PR. I know you are copying BasicTracerProvider.ts with minimal changes.
There was a problem hiding this comment.
New @opentelemetry/sdk-trace pacakge will have this export but it is not necessary to export this class right now.
There was a problem hiding this comment.
That commit stops exporting TracerProvider. I was talking about whether enum ForceFlushState needs to be exported. AFAICT it is only used internally in this file.
| const { XMLHttpRequestInstrumentation } = require('@opentelemetry/instrumentation-xml-http-request'); | ||
| const { WebTracerProvider } = require('@opentelemetry/sdk-trace-web'); | ||
|
|
||
| // XXX |
There was a problem hiding this comment.
Should this be removed or replaced?
| @@ -24,105 +23,30 @@ export enum ForceFlushState { | |||
| /** | |||
| * This class represents a basic tracer provider which platform libraries can extend | |||
There was a problem hiding this comment.
I suppose we won't consider dropping BasicTracerProvider in 3.0, because it is coming so soon?
If so, then yes, deprecating it some time after SDK 3.0 is released sounds good.
| import { AlwaysOnSampler } from './sampler/AlwaysOnSampler'; | ||
| import { RandomIdGenerator } from './platform'; | ||
|
|
||
| export enum ForceFlushState { |
There was a problem hiding this comment.
That commit stops exporting TracerProvider. I was talking about whether enum ForceFlushState needs to be exported. AFAICT it is only used internally in this file.
Which problem is this PR solving?
As mentioned in #5290 (comment) the Node SDK should resolve the tracer provider configuration from the SDK options or the environment if necessary. Also the
registermethod fromNodeTracerProvideris not used anymore because Node SDK already registers the propagators and the context manager using the api to register the globally.If the SDK resolved the config then the following classes are not needed
BasicTracerProviderbecause it has logic to get configuration from environment varsNodeTracerProviderbecause the only specific logic,registermethod, is unused.This PR is the 1st one to make SDK Node free from these classes and get it ready for a simple
TracerProviderclass which doesn't have hidden logic rather than the tracer provider creation.Refs: #5290
Refs: #6595
Short description of the changes
NodeTracerProviderandNodeTracerConfigas deprecatedBasicTracerProvideras deprecatedNodeTracerProvidertoBasicTracerProvider@opentelemetry/sdk-trace-nodeto point to@opentelemetry/sdk-trace-basesince they are just re-exports.TracerProviderclass which expects the config options resolved falling back to defaults from specTracerProviderOptionsinterface used in the constructor of the newTracerProviderclass.BasicTracerProviderbecomes a wrapper aroundTracerProviderclass providing a merged configuration from user and environmentType of change
Please delete options that are not relevant.
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
Checklist: