-
Notifications
You must be signed in to change notification settings - Fork 39
fix: bumped @opentelemetry/sdk-trace-base to v2 #2196
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
Conversation
a6dc436 to
694c185
Compare
637603a to
4388d65
Compare
| // NOTE: This assignment is necessary to display the database name in the UI. | ||
| // In the backend, for OpenTelemetry, the service name is based on the OpenTelemetry span attribute service.name. | ||
| if (otelSpan.attributes && 'db.name' in otelSpan.attributes) { | ||
| otelSpan.resource._attributes['service.name'] = otelSpan.attributes['db.name']; |
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.
otelSpan.resource._attributes no longer exists in v2.
I tested tedious manually. Works fine. I am not sure why we had to add this earlier, but seems like we don't need this anymore.
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.
Yes agree. This is something not needed as we already fixed the issue in wrap.js
Nice cleanup.
| if (!otelSpan || !otelSpan.instrumentationLibrary) { | ||
| // TODO: remove instrumentationLibrary in next major release | ||
| // instrumentationScope was introduced in OpenTelemetry v2 | ||
| if (!otelSpan || (!otelSpan.instrumentationScope && !otelSpan.instrumentationLibrary)) { |
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.
See open-telemetry/opentelemetry-js@97bc632
Leaving both names for now.
| expect(response.otelspan.instrumentationLibrary).to.be.an('object'); | ||
| expect(response.otelspan.instrumentationLibrary.name).to.eql('otel-sdk-app-tracer'); | ||
| expect(response.otelspan.instrumentationScope).to.be.an('object'); | ||
| expect(response.otelspan.instrumentationScope.name).to.eql('otel-sdk-app-tracer'); |
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.
I am not sure why this is not instrumentationLibrary 🤔
|
@copilot squash all commits into one with the following single commit fix: bumped @opentelemetry/sdk-trace-base to v2 |
aryamohanan
left a comment
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
e26f4f3 to
e45b5c9
Compare
|



refs https://jsw.ibm.com/browse/INSTA-66063
@opentelemetry/sdk-trace-base v2 supports >= 18.19.
We cannot merge that into main.