feat(mongodb): add db statement serializer config

[nozik]

Which problem is this PR solving?

• The payload for insert operations is currently explicitly excluded from being collected. It would be useful to collect it for monitoring, observability, security and many other use cases.

Short description of the changes

• Add a flag (disabled by default) that allows collecting the insert payload.

[blumamir]

wonder why insert statement was not collected vs update statement which always does. guess the intention was not to leak sensitive information via span attributes, but then it should probably include update as well?

[codecov]

Codecov Report

Merging #626 (86476d2) into main (aec1518) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #626      +/-   ##
==========================================
+ Coverage   96.68%   96.70%   +0.01%     
==========================================
  Files          13       15       +2     
  Lines         634      697      +63     
  Branches      124      128       +4     
==========================================
+ Hits          613      674      +61     
- Misses         21       23       +2     

Impacted Files	Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts	96.77% <0.00%> (ø)
...ages/auto-instrumentations-node/test/utils.test.ts	96.87% <0.00%> (ø)

[nozik]

@blumamir Sensitive information may appear anywhere - even in an HTTP request URL - and also in an update statement, as you mentioned

[blumamir]

@blumamir Sensitive information may appear anywhere - even in an HTTP request URL - and also in an update statement, as you mentioned

You are right, I don't know what was the initial intention for separating inset and update. I know in other instrumentations it was raised as an issue that instrumentation should be careful not to leak sensitive info via the db statement, but that ofcourse is no guarantee, only applying common sense as to where sensitive data more commonly reside.

With regards to the update-insert issue, I feel that adding a new instrumentation option that has the name insert in it may be something that will require a breaking change in the future if insert and update should be handled the same way.
Another approach that is implemented in ioredis and redis instrumentations is to use dbStatementSerializer option with a default implementation, which the user can override to include or format the statement attribute with custom logic according to the needs (a bit similar to the hooks that are used to apply customizations on attributes).

Anyway, these are all thoughts and suggestions, but other people might have different opinion :)

[nozik]

@blumamir I generally agree; I would only add that we're dealing with sensitive information anyway - it's unavoidable and has to be addressed somehow (e.g., provide a mechanism for doing that as part of OTEL, regardless of the source of the data). I tend to agree that the best solution would be breaking the existing behavior of the instrumentation and avoid the distinction between insert/update with a mechanism similar to the DB statement serializer., but let's hear other opinions on that :)

[vmarchaud]

I tend to agree that the best solution would be breaking the existing behavior of the instrumentation and avoid the distinction between insert/update with a mechanism similar to the DB statement serializer., but let's hear other opinions on that :)

Well i wrote this code and don't remember why i did this, i don't think it was to avoid leaking data since we already use enhancedDatabaseReporting for that. I would be in favor of removing the logic completly

[nozik]

@vmarchaud So I'll go ahead and remove this logic and add the ability to override the existing db statement serialization with a dbStatementSerializer config. Agreed?

[vmarchaud]

Agreed?

Yes :)

[nozik]

@vmarchaud Done :)

[obecny]

lgtm, one suggestion

[obecny]

I think you should do this checking a bit earlier for example

const dbStatementSerializer: DbStatementSerializer =
      typeof this._config.dbStatementSerializer === 'function' ? this._config.dbStatementSerializer :
      this._defaultDbStatementSerializer.bind(this);

[nozik]

done

[vmarchaud]

@nozik CI is failing because of an unused import:

src/instrumentation.ts:44:1 - error TS6133: 'DbStatementSerializer' is declared but its value is never read.

44 import { DbStatementSerializer } from './types';

[nozik]

@obecny @vmarchaud Done and done :)

[dyladan]

This is ready for merge but I don't have access to write to your branch so I can't update the PR to merge it. Please update the branch or give maintainers write acess to it.

[nozik]

@dyladan Done

[blumamir]

looks like lint now fails due to legitimate reasons.
@nozik - please run lint:fix to resolve

[nozik]

@blumamir 👍