ref(node-core): Vendor OTel rpc-metadata and drop @opentelemetry/core dependency#22274
ref(node-core): Vendor OTel rpc-metadata and drop @opentelemetry/core dependency#22274msonnb wants to merge 1 commit into
@opentelemetry/core dependency#22274Conversation
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c56eb18. Configure here.
| /** Set the current RPC metadata on the given context. */ | ||
| export function setRPCMetadata(context: Context, meta: RPCMetadata): Context { | ||
| return context.setValue(RPC_METADATA_KEY, meta); | ||
| } |
There was a problem hiding this comment.
Vendored RPC key breaks interop
High Severity
createContextKey returns a unique symbol on every call, so this vendored RPC_METADATA_KEY is not the same key as @opentelemetry/core's. Framework instrumentations (Express, Fastify, Koa, etc.) call getRPCMetadata from @opentelemetry/core and will no longer find the metadata httpServerSpansIntegration stores, so rpcMetadata.route stays unset and http.route / transaction-name updates from that path are lost for @sentry/node-core bring-your-own-OTel setups.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c56eb18. Configure here.
There was a problem hiding this comment.
this shouldn't be a problem since createContextKey calls Symbol.for under the hood. So if we keep the string exactly the same, this resolves to the same key. See https://github.com/open-telemetry/opentelemetry-js/blob/d8894cf99074d487203e1b814d9c3679019b63d3/api/src/context/context.ts#L13
|
I wonder if we need this at all? SInce we now control all of this, could we not get rid of this totally, and instead just directly set the things we need (http.route attribute I guess) directly? |
mhm so if i understand this correctly we can actually drop all the |
jup, but only in v11! for now we need to still set this as in theory external instrumentation could use this. I have slightly adjusted this here: #22292 |
… dependency Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c56eb18 to
3cb97a6
Compare
|
closing as we will just remove this in v11 |


@sentry/node-coreonly depended on@opentelemetry/corefor the RPC metadata helpers (getRPCMetadata/setRPCMetadata/RPCType) used by the HTTP server spans integration. This vendors that single small module and drops the dependency entirely.The vendored file is colocated with its sole consumer under
integrations/http/vendored/rpcMetadata.ts, matching the pattern already used for the vendored undici instrumentation (colocated with consumer) rather than a top-levelvendored/directory, since it has exactly one caller.With the import switched to the vendored copy,
@opentelemetry/coreis no longer referenced anywhere insrc, so it was removed frompeerDependencies,peerDependenciesMeta,devDependencies, and the README's dependency list and install commands.ref #21674
closes #22280