Uh oh!
There was an error while loading. Please reload this page.
ref(node): Streamline amqplib instrumentation - #21753
Conversation
| const modifiedOptions = options ?? {}; | ||
| modifiedOptions.headers = modifiedOptions.headers ?? {}; |
There was a problem hiding this comment.
Bug: The startPublishSpan function mutates the caller's options object by reference, adding tracing headers. This can cause unexpected side effects when the options object is reused.
Severity: MEDIUM
Suggested Fix
Defensively clone the incoming options object and its headers property to avoid mutating the original object. For example: const modifiedOptions = { ...(options ?? {}), headers: { ...(options?.headers ?? {}) } };.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/node/src/integrations/tracing/amqplib/vendored/utils.ts#L171-L172
Potential issue: The `startPublishSpan` function mutates the `options` object passed by
the caller. When a non-null `options` object is provided, it is assigned by reference to
`modifiedOptions`. Subsequent modifications to `modifiedOptions.headers` to add tracing
information (`sentry-trace`, `baggage`) directly alter the original user-provided
object. This is problematic as the `amqplib` library documentation encourages reusing
the `options` object for multiple publish calls. This undocumented side effect can lead
to unexpected behavior and data pollution in the user's application logic.
Did we get this right? 👍 / 👎 to inform future reviews.
size-limit report 📦
|
JPeer264
left a comment
There was a problem hiding this comment.
LGTM overall just one kinda nit
| return module; | ||
| } | ||
| private patchConnect(moduleExports: any): any { |
There was a problem hiding this comment.
l/m: There are a lot of any in this code, but linting doesn't fail - we should include this file again by removing it from the oxlintrc base.
There was a problem hiding this comment.
A lot of the already streamlined integrations are included in this rule - this is probably very much on purpose as the code uses a lot of module-patching and untyped library exports (so it's hard to type).
sentry-javascript/.oxlintrc.base.json
Lines 159 to 180 in fc39574
Streamlines the amqplib instrumentation:
startSpanAPIs from@sentry/coreinstead of the OTel tracer./* eslint-disable */from the vendored files so they pass the linter.Closes#20723
Linear: https://linear.app/getsentry/issue/JS-2375/streamline-opentelemetryinstrumentation-amqplib