Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invalid DBM Propagation Mode breaking mysql/mysql2 plugin #4140

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

choult
Copy link

@choult choult commented Mar 5, 2024

What does this PR do?

Fixes #4139

At time of writing, the documentation for the NodeJS tracing library on the DataDog website states that to switch off DBM Propagation in the tracing of database calls, the mode should be set to the value none.

This, however, leads to terminal hanging in the use of the mysql/mysql2 Node modules.

In debugging this, I determined that the documented value of none is no longer valid and the value disabled should be used instead. While this is clearly a documentation inconsistency, the fact that the plugin does not handle an unexpected value for mode leads to a break in functionality when dd-trace is upgraded from version 1.7, when this value was introduced.

This bug is caused by the unrecognized value - in this case none - falling through the database plugin's injectDbmQuery function without returning a query.

Therefore, this commit accommodatesthe old/documented value of none that disables propagation as well as defaults the behavior of the function to return the query unmolested in the event that an invalid value is used in future.

I could not see any unit tests for this, but this has been tested "in the wild" to success within my own test setups, and should be easily backported to previous versions.

Additional Notes

I also suggest that the documentation linked above is updated to reflect the "proper" value.

Security

Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

=============================================================

At time of writing, the
[documentation for the NodeJS tracing library](https://docs.datadoghq.com/database_monitoring/connect_dbm_and_apm/?tab=nodejs)
on the DataDog website states that to switch off DBM Propagation in the tracing of
database calls, the mode should be set to the value `none`.

This, however, leads to terminal hanging in the use of the mysql/mysql2 Node modules.

In debugging this, I determined that the documented value of `none` is no longer valid
and the value `disabled` should be used instead. While this is clearly a documentation
inconsistency, the fact that the plugin does not handle an unexpected value for `mode`
leads to a break in functionality when `dd-trace` is upgraded from version `1.7`, when
this value presumably changed.

This bug is caused by the unrecognized value - in this case `none` - falling through the
database plugin's `injectDbmQuery` function without returning a query.

Therefore, this commit accommodatesthe old/documented value of `none` that disables
propagation as well as defaults the behavior of the function to return the query
unmolested in the event that an invalid value is used in future.

I could not see any unit tests for this, but this has been tested "in the wild" to success
within my own test setups, and should be easily backported to previous versions.

I also suggest that the documentation linked above is updated to reflect the "proper"
value.
@choult choult requested a review from a team as a code owner March 5, 2024 22:23
@eugeneYWang
Copy link

related to #4139

@choult choult changed the title Fix invalid DBM Propagation Mode breaking mysql/mysql2 plugin Fix invalid DBM Propagation Mode breaking mysql/mysql2 plugin (#4139) Mar 5, 2024
@choult choult changed the title Fix invalid DBM Propagation Mode breaking mysql/mysql2 plugin (#4139) Fix invalid DBM Propagation Mode breaking mysql/mysql2 plugin Mar 5, 2024
@Qard
Copy link
Collaborator

Qard commented Mar 6, 2024

You can find DBM-related tests in each database plugin. For example, here's the mysql tests for DBM: https://github.com/DataDog/dd-trace-js/blob/master/packages/datadog-plugin-mysql/test/index.spec.js#L361

@choult
Copy link
Author

choult commented Mar 6, 2024

Perfect, thanks @Qard, will dig in properly then

kai-thomas-datadog added a commit to DataDog/documentation that referenced this pull request Mar 6, 2024
DataDog/dd-trace-js#4140
DataDog/dd-trace-js#4139

this is an issue for customer since a few versions ago, and the doc is causing confusion and errors.
cswatt pushed a commit to DataDog/documentation that referenced this pull request Mar 6, 2024
DataDog/dd-trace-js#4140
DataDog/dd-trace-js#4139

this is an issue for customer since a few versions ago, and the doc is causing confusion and errors.
…hould not be functionally different, and should let the SQL through
@choult choult requested a review from a team as a code owner March 27, 2024 16:17
@choult choult requested a review from jbertran March 27, 2024 16:17
@choult
Copy link
Author

choult commented Mar 27, 2024

@Qard I have been utterly unable to:

  • Obtain the couchbase server sandbox image from GHCR
  • Run yarn as per CONTRIBUTING.md

I have updated the MySQL tests to cover this scenario, hoping that they suffice.

@juan-fernandez juan-fernandez mentioned this pull request Apr 11, 2024
6 tasks
@juan-fernandez
Copy link
Collaborator

hey @choult, thanks for your contribution!

It looks like the lint job is failing: https://github.com/DataDog/dd-trace-js/actions/runs/8645760597/job/23703612890?pr=4140. Would you mind taking a look? 😄

@choult
Copy link
Author

choult commented Apr 12, 2024

hey @choult, thanks for your contribution!

It looks like the lint job is failing: https://github.com/DataDog/dd-trace-js/actions/runs/8645760597/job/23703612890?pr=4140. Would you mind taking a look? 😄

Well that's embarrassing - backticks for strings? I must have been in Markdown mode for half a second... moar coffeeeeee!

nathan-knight and others added 8 commits April 12, 2024 11:57
)

* Avoid run sequelize plugin test with non compatible mysql2

* Avoid run sequelize plugin test with non compatible mysql2

* Fix typo

* Fix typo

* Add comment with the test combination constraint explanation

* Update packages/dd-trace/test/appsec/iast/analyzers/sql-injection-analyzer.sequelize.plugin.spec.js

Co-authored-by: simon-id <simon.id@datadoghq.com>

---------

Co-authored-by: simon-id <simon.id@datadoghq.com>
* Emit an event when profiles are submitted
* Emit span start event
* Emit an app-closing event so telemetry users can publish final metrics
* SSI Telemetry class
* Telemetry mock profiler
Because commas are normalized to underscores in backend anyway.
@choult choult requested review from a team as code owners April 12, 2024 10:57
@choult choult requested a review from anmarchenko April 12, 2024 10:57
@choult choult requested a review from a team as a code owner May 7, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Official DD-tracer Document should be updated to point out none OF DBM option is replaced with value disabled