Update OpenTelemetry dependencies #876
Conversation
The Next.js instrumentation requires more recent versions of the `@opentelemetry/api` and `@opentelemetry/core` libraries. The instrumentation libraries are often locked to the minor version, so they need to be bumped as well. Also we get bugfixes and such.
|
✔️ All good! |
This fixes an issue in CI with the type definitions bundled with the `@opentelemetry/sdk-metrics` library. I couldn't really reproduce this issue locally. Also bump the versions of some other things that depend on the TypeScript version or in each other.
unflxw
force-pushed
the
bump-otel-dependencies
branch
from
c1d7775 to
4172c3a
Compare
|
After having to upgrade to TypeScript 5.0 in order to solve some I'll look into those on Monday. Marking this as a draft for now. |
unflxw
force-pushed
the
bump-otel-dependencies
branch
2 times, most recently
from
e62e637 to
bcd2cdc
Compare
| @@ -376,7 +376,7 @@ export class Client { | |||
| spanProcessor | |||
| }) | |||
|
|
|||
| this.instrumentationsLoaded = sdk.start() | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
sdk.start() no longer returns a Promise<void>, just a good ol' void. I'm taking this to mean that the SDK is now loaded synchronously. I need to double-check that assumption before merging.
Before this function is called, the client's initialiser already sets instrumentationsLoaded to Promise.resolve(). So await client.instrumentationsLoaded now becomes an unnecessary no-op, but it won't break for users who have added it to their code.
If this is the case and there really is nothing that needs to be awaited on the SDK's initialisation, then we should probably remove the documentation for instrumentationsLoaded and deprecate it for removal in v4. We could also put it behind an accessor that emits a warning, telling users that they no longer need to call it.
There was a problem hiding this comment.
Looked at the change to the SDK start. Here's the relevant PR and the issue it solved -- the issue being the same one we were encountering, which we addressed by exposing the promise as instrumentationsLoaded. So the changes in this PR are correct.
I have therefore created an issue for deprecating instrumentationsLoaded.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
While performing the daily checks some issues were found with this Pull Request.
|
Calling `sdk.start()` no longer returns a promise, so assigning it to `client.instrumentationsLoaded` is a type error. Instead, when the client is initialised, `client.instrumentationsLoaded` will be an already-resolved promise.
We're already using it as if it was. I get these compilation errors locally, but not on CI. Not sure what that's about.
TypeScript 5.0 seems to (correctly) ask you to cast the error to `Error` in order to pass it to a function of that kind.
The HTTP spans no longer start with "HTTP" (so, instead of "HTTP GET", it just says "GET") and the HTTP spans wrapping the Apollo and Yoga spans now have `POST /graphql` in their span name. This seems like a good change. The PostgreSQL spans now have the name of the DB after the span name, and their name more closely resembles a span category. In addition, there are now spans that represent the connection. We should amend the extractor to better deal with these spans, but it's okay for now.
unflxw
force-pushed
the
bump-otel-dependencies
branch
from
bcd2cdc to
c01b5aa
Compare
The Next.js instrumentation requires more recent versions of the
@opentelemetry/apiand@opentelemetry/corelibraries. The instrumentation libraries are often locked to the minor version, so they need to be bumped as well. Also we get bugfixes and such.