feat!: Bump to OpenTelemetry v2#16872
Conversation
size-limit report 📦
|
|
|
||
| // Access the span processors from the provider via _activeSpanProcessor | ||
| // Casted as any because _activeSpanProcessor is marked as readonly | ||
| const multiSpanProcessor = (provider as any)._activeSpanProcessor as |
There was a problem hiding this comment.
not sure, but sometimes it also works to do provider['_activeSpanProcessor'] to make TS happy!
andreiborza
force-pushed
the
ab/bump-otel-v2
branch
11 times, most recently
from
6de3296 to
957a4c8
Compare
andreiborza
force-pushed
the
ab/bump-otel-v2
branch
2 times, most recently
from
8c43c8a to
213aa83
Compare
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Nice! I just had a couple of questions regarding changed attributes/data mostly. Otherwise LGTM!
| data: { | ||
| 'express.name': 'query', | ||
| 'express.type': 'middleware', | ||
| 'http.route': '/', |
There was a problem hiding this comment.
logaf-l but our of curiosity: why was this attribute removed?
There was a problem hiding this comment.
The express instrumentation doesn't add it anymore to middleware spans, I didn't find anything in their changelog regarding this tho so it's more of an observation after running it 😅.
There was a problem hiding this comment.
Actually, this likely came from open-telemetry/opentelemetry-js-contrib#2843. Looks like it might be a bug in the instrumentation now because these spans are not for a 404 route..
There was a problem hiding this comment.
@s1gr1d could you take a look at that please?
There was a problem hiding this comment.
This was a bug before, as it was always attaching http.route. http.route should not be included when no route was matched (on 404).
The matched route, that is, the path template in the format used by the respective server framework.
Semantic Convention Docs: https://opentelemetry.io/docs/specs/semconv/registry/attributes/http/
But in this case it's not a 404 route 🤔
There was a problem hiding this comment.
Just checked: Middleware spans might not include the http.route attribute when it's not yet known at the time the middleware runs. And 'http.route': '/' is only added when there is an explicitly defined root route AND when when the route resolution is already done (middleware might run before that).
And why it was added before: Previously, the attribute ALWAYS defaulted to / but this was wrong. https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2843/files#diff-81c3e13c3dac53e0dd6aabf6dc165305350eb522a85a2561f9ee51d9249011e7L198
There was a problem hiding this comment.
Awesome, thanks so much for investigating and fixing this upstream @s1gr1d :)
There was a problem hiding this comment.
ok, no attribute sounds better than a wrong attribute. Thanks for the explanation!
dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
Show resolved
Hide resolved
...applications/react-router-7-framework-node-20-18/tests/performance/trace-propagation.test.ts
Show resolved
Hide resolved
packages/nestjs/package.json
Outdated
| "@opentelemetry/instrumentation-nestjs-core": "0.44.1", | ||
| "@opentelemetry/core": "^2.0.0", | ||
| "@opentelemetry/instrumentation": "^0.202.0", | ||
| "@opentelemetry/instrumentation-nestjs-core": "0.48.1", |
There was a problem hiding this comment.
We can then switch this one in a separate PR (replacing our vendored instrumentation)
There was a problem hiding this comment.
We have to switch anyway to match deps, it shouldn't affect our vendored instrumentation tho.
andreiborza
force-pushed
the
ab/bump-otel-v2
branch
2 times, most recently
from
2105d24 to
6ee8623
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
andreiborza
force-pushed
the
ab/bump-otel-v2
branch
from
6ee8623 to
f7b894f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
andreiborza
force-pushed
the
ab/bump-otel-v2
branch
from
f7b894f to
f3fcd27
Compare
5 participants
This PR
0.202.0for experimental and latest versions for all instrumentations).@sentry/opentelemetryOpenTelemetry v2 test dev-package as this is now covered by the package itselfNote: Once this PR is merged, the
developbranch officially becomes thev10branch and any releases for v9 have to happen on thev9branch.