-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[javascript] discontinue use of deprecated APIs #6416
[javascript] discontinue use of deprecated APIs #6416
Conversation
32993f6
to
ae862b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from a copy edit perspective. Let's get another set of eyes on this from @open-telemetry/javascript-approvers. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLGTM
@@ -178,10 +178,9 @@ const FrontendTracer = async () => { | |||
resource: new Resource({ | |||
[SEMRESATTRS_SERVICE_NAME]: process.env.NEXT_PUBLIC_OTEL_SERVICE_NAME, | |||
}), | |||
spanProcessors: [new SimpleSpanProcessor(new OTLPTraceExporter())], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be the snippet of FrontendTracer.ts
.
But it seems it is already out-of-date, so I won't block this PR.
I've created the issue open-telemetry/opentelemetry-demo#2080 to follow-up on the Demo repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pichlermarc - so this is gating on open-telemetry/opentelemetry-js#5148? I'm going to mark this as blocked and "request changes" to ensure that this doesn't get merged ahead of time by mistake.
I think the two API PRs can be merged once they're approved based on the tracking issue. So I don't think they need to be blocked. But @pichlermarc can confirm.
|
Thanks for double-checking. 🙂 open-telemetry/opentelemetry-js#5148 is not required before this PRcan be merged. All the features used here are available in the latest 1.x version of the SDK. After this PR, I will follow up with another one that will actually lift the docs to 2.0 (that one will then be blocked by open-telemetry/opentelemetry-js#5148) 🙂 |
Hi all - this PR contains updates to documentation discontinuing the use of deprecated APIs. As discussed in #6396 this is in preparation of the upcoming SDK
2.0.0
release, but uses only features that are available in the currently latest release1.30.1
/0.57.2
for now.I will follow up with 2 other PRs for
assets/js/tracing.js
(edit: refactor(tracing): discontinue use of deprecated APIs #6418)Notable changes:
BasicTracerProvider#register()
will be removed to allow for better tree-shakingBasicTracerProvider#register()
NodeTracerProvider#register()
is still available in 2.0, therefore docs using it remain unchangedWebTracerProvider#register()
is still available in 2.0, therefore docs using it remain unchanged*TracerProvider#addSpanProcessor()
was replaced by aspanProcessors
constructor option, which is available since1.28.0
Refs #6396