fix: unified OpenTelemetry API instance to prevent duplicate instances #2054
Conversation
aryamohanan
force-pushed
the
build-opentelemetryinstrumentationfs-0260
branch
3 times, most recently
from
e103c73 to
545ac45
Compare
aryamohanan
force-pushed
the
build-opentelemetryinstrumentationfs-0260
branch
2 times, most recently
from
5c8f56d to
8d4ea7e
Compare
aryamohanan
force-pushed
the
build-opentelemetryinstrumentationfs-0260
branch
from
af14bde to
cedf865
Compare
|
A few tests are failing, specifically, the ones that assert Root Cause: Multiple OTEL API instances are being loaded that intercept the tracing Working case: When modules are loaded from the same location (packages/core/node_modules), both the core code and the FS instrumentation share a single OTEL API instance. As a result, spans are created correctly. Non-working case: When the FS instrumentation is loaded from a different location (e.g., root node_modules), it uses a separate OTEL API instance. Since our core code applies monkey patches to its own API instance(packages/code/node_modules), the instrumentation does not receive those patches, causing spans to be missing and tests to fail. |
aryamohanan
force-pushed
the
build-opentelemetryinstrumentationfs-0260
branch
from
c3fef76 to
70473cf
Compare
aryamohanan
force-pushed
the
build-opentelemetryinstrumentationfs-0260
branch
2 times, most recently
from
0949a9d to
fc8f9a2
Compare
| "overrides": { | ||
| "debug": "^4.4.3" | ||
| }, | ||
| "peerDependencies": { |
There was a problem hiding this comment.
Added @opentelemetry/api as a peer dependency with a bounded version range (>=1.3.0 <1.10.0) to ensure our package uses the same global API instance as the application and instrumentations. This prevents duplicate API instances, which could otherwise result in NonRecordingSpan and make our monkey patch fail.
The version bounds also guarantee compatibility with the APIs our patch relies on while avoiding breaking changes from newer versions.
There was a problem hiding this comment.
You removed the "@opentelemetry/api": "1.9.0" dep.
You can't define this only as peer dep? We have to install our version.
Otherwise how this work with customers who don't have any other otel deps?
There was a problem hiding this comment.
if @opentelemetry/api is declared only as a peer dependency, it won’t be automatically installed in older npm versions (v3–v6). However, starting from npm v7, peer dependencies are installed by default.
see: https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependencies
Since Node.js v18 ships with npm v8.6, it’s safe to use this as a peer dependency only.
I’ve intentionally kept it as a peer dependency to ensure that both the application and our package share the same global OpenTelemetry API instance. Tested locally and verified.
This approach is also the one recommended by the OpenTelemetry maintainers.
There was a problem hiding this comment.
By default, npm will pick the latest version below 1.10.0, currently 1.9.0 and install it.
There was a problem hiding this comment.
Oh they changed this behavior.
In npm versions 3 through 6, peerDependencies were not automatically installed, and would raise a warning if an invalid version of the peer dependency was found in the tree. As of npm v7, peerDependencies are installed by default.
https://github.blog/news-insights/product-news/npm-7-is-now-generally-available/#peer-dependencies
Hm I have to think about this.
There was a problem hiding this comment.
- Customer sets "@opentelemetry/api: 1.2.0" and is using an older Opentelemetry setup. Our peerDependency is "> 1.3.0". Could you please check what will happen? Our SDK should not make the customers installation fail.
- How do you wanne manage the upper bound rule? (<1.10.0)
There was a problem hiding this comment.
Customer sets "@opentelemetry/api: 1.2.0" and is using an older Opentelemetry setup. Our peerDependency is "> 1.3.0". Could you please check what will happen? Our SDK should not make the customers installation fail.
If a customer is using an older version @opentelemetry/[email protected], npm will block the installation by default and show a dependency resolution error, since 1.2.0 does not satisfy the required version range, but If we want to ensure installation does not fail hard, we could include @opentelemetry/api as a dependency as well 👍
How do you wanne manage the upper bound rule? (<1.10.0)
We can manage this by updating the version range whenever a new minor or major release of @opentelemetry/api is published, or earlier if a user reports an incompatibility or feature request. they don't release api so often.
I referred this: https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-base/package.json#L87
aryamohanan
changed the title
aryamohanan
force-pushed
the
build-opentelemetryinstrumentationfs-0260
branch
from
129491f to
31cbdc2
Compare
aryamohanan
force-pushed
the
build-opentelemetryinstrumentationfs-0260
branch
from
21be414 to
f3fedf8
Compare
| pid: String(serverControls.getPid()), | ||
| dataProperty: 'tags', | ||
| extraTests: span => { | ||
| expect(span.data.tags.name).to.contain('test_reply receive'); |
There was a problem hiding this comment.
They have updated the span name, see https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2495/files
|
3 participants
Changes
Load @opentelemetry/api from the app root to ensure all instrumentations and monkey patches share the same global API instance.
Add peer dependency with bounded version range (>=1.3.0 <1.10.0) to enforce compatibility and avoid runtime issues like NonRecordingSpan.
Updated all @opentelemetry instrumentations versions to latest
References:
https://github.com/open-telemetry/opentelemetry-js/issues/4832
open-telemetry/opentelemetry-js#5454
https://jsw.ibm.com/browse/INSTA-59002