[otelmongo] Disable adding the mongo 'db.statement' tag by default

[dubonzi]

As of now, the 'db.statement' tag is not obfuscated, which can lead to sensitive information being leaked through the tag.

See #3388.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: dubonzi / name: Eduardo Bonzi da Conceição (b978ed7)

[codecov]

Codecov Report

Merging #3519 (419c4e7) into main (6682591) will increase coverage by 16.2%.
The diff coverage is 100.0%.

Additional details and impacted files

@@           Coverage Diff            @@
##            main   #3519      +/-   ##
========================================
+ Coverage   70.1%   86.4%   +16.2%     
========================================
  Files        147       4     -143     
  Lines       6973     125    -6848     
========================================
- Hits        4892     108    -4784     
+ Misses      1958      14    -1944     
+ Partials     123       3     -120     

Impacted Files	Coverage Δ
....mongodb.org/mongo-driver/mongo/otelmongo/mongo.go	86.8% <ø> (+86.8%)	⬆️
...mongodb.org/mongo-driver/mongo/otelmongo/config.go	100.0% <100.0%> (+100.0%)	⬆️

... and 146 files with indirect coverage changes

[dmathieu]

Thank you! 🌷
This will require a changelog entry.

[dubonzi]

Done!

[dmathieu]

(sorry for the back and forth)

I think we should update the comment in

opentelemetry-go-contrib/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo/mongo.go

Line 110 in 3ffe346

// TODO sanitize values where possible

to say something like:

// TODO sanitize values where possible, then reenable `db.statement` span attributes default.

[pellared]

Personally, I agree that we should be "secure by default".
Unfortunately, it is currently against the OTel specification.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#call-level-attributes

Can you first try addressing it in the OTel specification? I would support it 😉

Related PR in spec: open-telemetry/opentelemetry-specification#1659

[pellared]

Related issue in OTel spec: open-telemetry/opentelemetry-specification#3104

[dubonzi]

I added a comment in support of being able to have it disabled by default if the instrumentation doesn't implement obfuscation.

[dubonzi]

I was thinking about trying to implement obfuscation and remembered that the datadog agent does it.

Took a look at the code and doesn't seem like an easy task. Hopefully the specification changes to allow for removing the tag by default.

[pellared]

@dubonzi There is also an open PR here: open-telemetry/opentelemetry-specification#3127

[dubonzi]

Related issue in OTel spec: open-telemetry/opentelemetry-specification#3104

@dmathieu I see you tried to link to this PR in a comment but you linked to the wrong one. I edited my comment to include the correct link.

[dubonzi]

While the specification issue seems to be moving forward, I was thinking, would it be acceptable to use datadog's obfuscation library that is used in their agent to do the sanitization?

[pellared]

While the specification issue seems to be moving forward, I was thinking, would it be acceptable to use datadog's obfuscation library that is used in their agent to do the sanitization?

Obfuscation on "raw data" is never perfect. We could use similar feature as opt-in, but we should not use it by default.

EDIT:
The reason is similar to

It is not recommended to attempt any client-side parsing of db.statement just to get these properties, they should only be used if the library being instrumented already provides them.

from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md

[dubonzi]

Can we move forward with this now that the specification issue was accepted?

[pellared]

Side note: this package has no unit tests 😬