Update OpenTelemetry dependencies #964
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✔️ All good! |
|
@unflxw can you fix the build and add a changeset? |
unflxw
force-pushed
the
update-opentelemetry-dependencies
branch
from
6878c50 to
12b31a9
Compare
The test apps are at their most useful when testing against the latest versions of our dependencies, which are the ones that will be installed by default when running `npm install @appsignal/nodejs`. Add a script to update the dependencies for all test apps at once.
unflxw
force-pushed
the
update-opentelemetry-dependencies
branch
2 times, most recently
from
a91d24d to
ac047d0
Compare
Accidentally using several `@opentelemetry/api` minor versions, which is almost inevitable due to how npm resolves dependencies, triggers an absolutely deranged "feature" in the OpenTelemetry API internals, by which the global tracer set by the OpenTelemetry API version with the lower version number will not be visible by the one with the higher version number, and an error message about it will be logged to nowhere. This is not helped by the fact that `@prisma/instrumentation` has a hardcoded version bound, which they bump automatically for no reason whatsoever. Do a best-effort attempt to force OpenTelemetry versions to a specific version range instead, by picking versions of the instrumentations that are compatible with the `@opentelemetry/api` minor version. This will hopefully trick OpenTelemetry into picking versions that it can dedupe. Maybe. Who knows. We'll have to remember to update this in the future, whenever the release tracker tells us to. If this breaks again in the future, let's tighten the version bounds further.
Fix the bounds to point to `@appsignal/nodejs` version 3, and relax the bounds on other dependencies.
unflxw
force-pushed
the
update-opentelemetry-dependencies
branch
from
ac047d0 to
749cd3e
Compare
Update the `package-lock.json` files in the integration test apps in order to bring in newer versions of their dependencies.
This ensures that the fix for #928 is present.
It might be best to let this dependency's version be picked transitively via `@opentelemetry/sdk-trace-node`.
Because `npm link` transitive dependencies don't really work like other transitive dependencies, add the `@opentelemetry/api` dependency back to the `express-redis` integration test app, with the same version bound as the one hardcoded in the package.json. (Actual users of `@appsignal/nodejs` do not need to do this)
unflxw
force-pushed
the
update-opentelemetry-dependencies
branch
from
749cd3e to
6d3f4b1
Compare
|
@tombruijn build fixed. Not sure what the changeset would be, this is just maintenance work. |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
@unflxw In this case it can be something like "Updated OpenTelemetry dependencies with fix for Next.js and Webpack compatability" and other important fixes we know these updates include. |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This is a message from the daily scheduled checks. |
We want to release these changes and this requires a changeset.
tombruijn
force-pushed
the
update-opentelemetry-dependencies
branch
from
86a2c9e to
02dda5b
Compare
For some reason in the merge commit with the main branch it installed the wrong versions of packages and I got a linter issue on one of the files in the package. Rebuild the `package-lock.json` file (by removing `node_modules` and `package-lock.json`, and running `npm install`) and commit the result.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #928. The test dependencies are updated to make sure that this actually works.
Update OpenTelemetry dependencies
Update dependencies in our
package-lock.jsonto newer versions.Update the dependency bound for
@opentelemetry/sdk-nodeto^0.45.0to ensure it brings in the fix for #928.Add script to update test app dependencies
The test apps are at their most useful when testing against the
latest versions of our dependencies, which are the ones that will
be installed by default when running
npm install @appsignal/nodejs.Add a script to update the dependencies for all test apps at once.
Update test app dependencies
Update the
package-lock.jsondependencies on the test apps toensure that AppSignal works with the latest
@opentelemetry/sdk-node.