ref(node): Streamline mongoose instrumentation#21481
Conversation
Replace the OpenTelemetry tracing APIs in the vendored mongoose instrumentation with Sentry primitives (startInactiveSpan, getActiveSpan, span.setStatus) and bake the span origin in directly. Also drops the config paths that are dead in Sentry's context (dbStatementSerializer, requireParentSpan, suppressInternalInstrumentation) and the SemconvStability dual-emission machinery, keeping only the OLD attribute set. The OpenTelemetry instrumentation base/module-patching layer is kept.
With the OTel responseHook/dbStatementSerializer paths removed, the only config the SDK uses is the base InstrumentationConfig. Collapse MongooseInstrumentationConfig to that and drop the now-unused SerializerPayload, DbStatementSerializer, ResponseInfo and MongooseResponseCustomAttributesFunction types.
Remove ATTR_DB_STATEMENT (fed the removed dbStatementSerializer path) and DB_SYSTEM_NAME_VALUE_MONGODB (only used by the dropped stable-semconv branch). The remaining constants are the OLD attribute set the instrumentation still emits.
Port the upstream OTel mongoose test coverage as unit tests that drive a fake mongoose module through MongooseInstrumentation and assert the produced Sentry spans. Covers save, query exec, aggregate, insertMany, bulkWrite, document update methods (v8.21+), remove (v5/v6), error status, parent-span linking and unpatch.
Type the vendored instrumentation's `module` parameter (MongooseModule / MongooseModuleExports) instead of `any`, so the patch/unpatch logic is checked against the mongoose shape. Adjust lint config accordingly.
…ion suite Exercise more mongoose operations against the real mongodb-memory-server so the live suite asserts span output (name, OLD attributes, op, origin) for aggregate, insertMany and bulkWrite in addition to save/findOne.
logaretm
force-pushed
the
awad/js-2389-streamline-opentelemetryinstrumentation-mongoose
branch
from
b56fa12 to
f5972de
Compare
| "typescript/no-explicit-any": "off", | ||
| "no-unsafe-member-access": "off", | ||
| "no-this-alias": "off" |
There was a problem hiding this comment.
These are very common in instrumentations, so I wanted to remove the blanket eslint-disable we have and instead target the problematic rules.
size-limit report 📦
|
logaretm
requested review from
JPeer264 and
andreiborza
and removed request for
a team
| attributes, | ||
| parentSpan, | ||
| ); | ||
| const span = self._startSpan(this._model.collection, this._model?.modelName, 'aggregate', parentSpan); |
There was a problem hiding this comment.
can we possibly rewrite this to use startSpan(..., () => callback) or a similar thing? Using our active span wrapper has some benefits as that has built in error handling etc 🤔
There was a problem hiding this comment.
I noticed the initial implementation used inactive spans so I didn't want to change semantics here and opted to use the startInactiveSpans to keep things as they are, minimal changes and all of that.
Now, I don't really see a lot of spans getting affected by switching to startSpan. The only thing that would run inside mongoose spans would be a mongodb driver calls, so I believe that is a good thing for them to be parented to it.
I will try switching to a mix of startSpan and startSpanManual (for callback-based paths) and see if that works.
There was a problem hiding this comment.
It changes more than I expected, so we change some error messages (which we can redecorate) but then there is the thenable issue....
Some methods, like 8.21+ document's updateOne/deleteOne don't return a real Promise, they return a lazy Query. The instrumentation has to hand that Query back to the caller un-executed (we just tag it so the eventual .exec() isn't instrumented twice).
This interacts badly with startSpan/startSpanManual as handleCallbackErrors calls .then() on anything thenable, and for a mongoose Query, .then() would execute it. So wrapping these in startSpan runs the query prematurely.
I tested this against mongoose 8.21 and it did produce this issue, so it seems like we can't do that. I will add tests for these cases since our CI wouldn't have caught it.
For parenting I could just do withActiveSpan but that's just a single call change.
Wrap the mongoose operation in `withActiveSpan` so the span is active while the underlying driver call runs. This parents the mongodb driver spans under the corresponding mongoose span (e.g. mongoose.User.save -> mongodb insert) instead of leaving them as siblings. `withActiveSpan` returns the callback result untouched, so it does not call `.then()` on lazy mongoose Query thenables (unlike startSpan/startSpanManual, which would execute them prematurely). The active window is synchronous-only, so unrelated spans are not parented to the mongoose span.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Reviewed by Cursor Bugbot for commit 66ac64a. Configure here.
| kind: SpanKind.CLIENT, | ||
| attributes, | ||
| parentSpan, | ||
| }); |
There was a problem hiding this comment.
Missing mongoose span op
Medium Severity
Mongoose instrumentation creates spans via startInactiveSpan with origin and DB attributes but never sets sentry.op. After switching from OpenTelemetry spans (where the exporter inferred db from semantic conventions), finished spans and transaction children no longer get op: 'db', which breaks integration expectations and Sentry span categorization.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 66ac64a. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Add integration suites pinning mongoose 7, 8 and 9 (alongside the existing v6 suite) so every supported version branch is exercised against a real mongoose: contextCaptureFunctions7 (v7), the 8.21+ document-method path (v8) and the latest major (v9). The v8 suite exercises document `updateOne`/`deleteOne`, whose lazy Query must be handed back un-executed. A unit test additionally guards this directly: it asserts the instrumentation never calls `.then()` on the returned thenable Query (which `startSpan`/`startSpanManual` would do, executing the query prematurely).
logaretm
force-pushed
the
awad/js-2389-streamline-opentelemetryinstrumentation-mongoose
branch
from
ee989f4 to
d0fb03f
Compare
2 participants
Streamlines the vendored
mongooseinstrumentation to use Sentry's span APIs instead of the OpenTelemetry tracing API, and removes the code paths that are dead in Sentry's context.I also ported OTel's own integration tests of mongoose and added my own assertions to it.
I didn't fix the older semantic attributes as I believe that would be a breaking change right now.
Fixes #20737