Circular reference in _scrubStatement causes maximum call stack exceeded in instrumentation-mongodb

[tzjames]

I am using mongoose which seems to have added a __parentArray, to an object that is part of an array. Hence there is a circular reference and so _scrubStatement then throws:
@opentelemetry/instrumentation-mongodb Error running dbStatementSerializer hook RangeError: Maximum call stack size exceeded at /Users/james/Library/CloudStorage/Dropbox/growthbookzip/growthbook/node_modules/@opentelemetry/instrumentation-mongodb/build/src/instrumentation.js:581:65 at Array.map (<anonymous>) at MongoDBInstrumentation._scrubStatement

This was introduced in instrumentation-mongodb.

You probably need to add a circular reference check. Or some hard coded check for __parentArray, since I'm assuming mongoose is pretty common.

What version of OpenTelemetry are you using?

    "@opentelemetry/api": "^1.9.0",
    "@opentelemetry/auto-instrumentations-node": "^0.47.1",
    "@opentelemetry/exporter-metrics-otlp-proto": "^0.52.0",
    "@opentelemetry/sdk-metrics": "^1.25.0"

What version of Node are you using?

v20.12.2

What did you do?

Ran some DB query that got automatically scrubbed.

My mongoose schema has a property phases that is defined as:

 phases: [
    {
      _id: false,
      dateStarted: Date,
      dateEnded: Date,
      phase: String,
      name: String,
      reason: String,
      coverage: Number,
      condition: String,
      savedGroups: [
        {
          _id: false,
          ids: [String],
          match: String,
        },
      ],
      prerequisites: [
        {
          _id: false,
          id: String,
          condition: String,
        },
      ],
      namespace: {},
      seed: String,
      variationWeights: [Number],
      groups: [String],
    },
  ],

It probably was a create statement.

What did you expect to see?

No Errors.

What did you see instead?

The maximum call stack error above.

Additional context

None

[tzjames]

@Fox32 Pinging you since I think you introduced it in your PR a week ago. Pinging @david-luna as well as you may have context here having approved it.

Thanks for your help!

[Fox32]

Trying to wrap my head around what is happening. Can you provide an example? I wonder how MongoDB itself is handling that when serializing the statement.

[david-luna]

@tzjames

Is it possible to reproduce by checking out the source branch of this PR and run the app?

[tzjames]

@david-luna Yes. Sorry. I was trying at one point to make a simpler app to reproduce it but the api_1.context.active() was not being set for it. So I failed.

But yes it is reproducible if you checkout the ji/monitor_job_queue branch of the growthbook/growthbook repo.
Then get Mongo up and running with:

docker run -d -p 27017:27017 --name mongo \
  -e MONGO_INITDB_ROOT_USERNAME=root \
  -e MONGO_INITDB_ROOT_PASSWORD=password \
  -v ~/gb_mongo_data/:/data/db \
  mongo

then run yarn and yarn build and yarn start:with-tracing.
Go to localhost:3000
Create a new org
Go to http://localhost:3000/getstarted/experiment-guide
click "View sample experiment"
Click the three dots next to "stop experiment" in the upper right and click 'edit status' and change the dropdown to 'draft'.
Click "start experiment".
Click "start anyway"
then in the logs you should see the Maximum call stack size exceeded.

[tzjames]

For what it's worth here is code within Mongoose that handles this for the spread operator on docs: https://github.com/Automattic/mongoose/blob/e4462c6c80b8b395187b382f7dcb3c398293cd78/lib/helpers/document/handleSpreadDoc.js#L5

ScrubStatement is not using the spread operator but perhaps similar code is necessary.

[alko89]

Is there a workaround on this?

[DanStory]

Is there a workaround on this?

@alko89 We have a different issue with the newly introduced _scrubStatement and using a workaround by providing our own dbStatementSerializer which would no longer call that method.

[trentm]

Some thoughts on this after poking around a bit. Caveat: I don't use MongoDB or really know it that well.

• semconv for mongodb spans (https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/mongodb.md) doesn't cover db.query.text (the new name for db.statement). The only reference is an example span where db.query.text: not set.

• General semconv discussion of semconv spans says to do what is being done by the scrub function (https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-spans.md):

  The db.query.text SHOULD be collected by default only if there is sanitization that excludes sensitive information. Sanitization SHOULD replace all literals with a placeholder value. Such literals include, but are not limited to, String, Numeric, Date and Time, Boolean, Interval, Binary, and Hexadecimal literals.

• It would, I suppose, be nice to skip document properties added by mongoose: https://github.com/Automattic/mongoose/blob/b34aba65bd64540e330665477f542eb79c877909/lib/helpers/document/compile.js#L213-L235
  However, it would be good to understand if why the scrub function was iterating over __parentArray, because the comment for that code in mongoose suggests it makes those property descriptors enumerable = false.

• A depth-limiter on the scrubbed document sounds good. DD's impl limits to a depth of 10, FWIW. (If I read it correctly, DD's impl also skips arrays, so collection.aggregate(...) usage wouldn't include a db statement.)

• It would be great to get a real code example (for tests), possibly from instrumentation-mongodb: performance degradation due to _scrubStatement #2516, to test the scenario with large binary data there. Was it a Buffer? A Uint8Array? Some other type?

• Perhaps the scrub could have a guard on instanceof Uint8Array (which also catches Buffer instances, at least in node 18), as is used here: https://github.com/open-telemetry/opentelemetry-js/blob/e97cc2ee8efa176232dc701462a49e9a10e95dfc/experimental/packages/otlp-transformer/src/common/internal.ts#L48

• These document objects that we're scrubbing feel like the wildwest from various examples that I've seen.