contrib/database/sql: prevent DBM propagation full mode with incompatible dbs #2328

rarguelloF · 2023-11-02T12:59:56Z

What does this PR do?

Automatically downgrade DBM propagation mode from full to service mode with an incompatible database (currently SQL Server and Oracle).

Motivation

These DBs don't support full propagation mode due to statement caching behavior which could cause performance issues when including full trace context. See Datadog official docs for more info: https://docs.datadoghq.com/database_monitoring/connect_dbm_and_apm

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
There is a benchmark for any new code, or changes to existing code.
If this interacts with the agent in a new way, a system test has been added.

For Datadog employees:

If this PR touches code that handles credentials of any kind, such as Datadog API keys, 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!

contrib/database/sql/sql.go

pr-commenter · 2023-11-02T13:14:15Z

Benchmarks

Benchmark execution time: 2023-12-01 10:24:08

Comparing candidate commit 4c9ef99 in PR branch rarguelloF/dbm-prevent-full-mode with baseline commit d2016f2 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 41 metrics, 0 unstable metrics.

…vers

`

vandonr · 2023-11-02T13:52:40Z

contrib/database/sql/sql.go

+	fullModeSupported := func() bool {
+		unsupportedDrivers := []string{"sqlserver", "oracle"}
+		for _, dr := range unsupportedDrivers {
+			if dr == driverName {


this looks a bit brittle, tell me if I'm wrong, but it looks like driverName is just a key used to retrieve it later and/or make sure init is only done once, and it can take pretty much any value ?

Obviously take my perspective with a grain of salt since I'm not an owner/maintainer for the go tracer but I agree that the driverName based logic is somewhat brittle. I know it's done in other places but I wonder if we could still consider doing type checking on the driver.Driver or driver.Connector.

Something like

if connector.Driver().(go_ora.OracleDriver) { // we know this is one of the oracle drivers }

Agree! I've added a new way to detect the driver, following this logic:

check driver package path using reflection

check dsn prefix

lastly check the driverName like I was doing in this version

please let me know what you think @vandonr @alexandre-normand

alexandre-normand · 2023-11-02T16:00:53Z

contrib/database/sql/sql.go

+		return true
+	}
+	if cfg.dbmPropagationMode == tracer.DBMPropagationModeFull && !fullModeSupported() {
+		log.Warn("Using DBM_PROPAGATION_MODE in 'full' mode is not supported for %s. See "+


I think it might make sense here to state that we're downgrading to service mode.

alexandre-normand · 2023-11-02T16:01:51Z

contrib/database/sql/sql.go

+}
+
+func checkDBMPropagation(cfg *config, driverName string) {
+	fullModeSupported := func() bool {


Just curious about style: why make this an inner anonymous function rather than a named function?

it was just a matter of namespacing (this way I could use a shorter function name instead of dbmPropagationFullModeSupported). Anyway, I refactored this code, please take a look at the new version 🙏

alexandre-normand · 2023-11-02T16:04:07Z

contrib/database/sql/propagation_test.go

@@ -288,6 +288,33 @@ func TestDBMTraceContextTagging(t *testing.T) {
 	}
 }

+func TestDBMPropagation_PreventFullMode(t *testing.T) {


It might make sense to have a test that goes through the implicit driver registration (i.e. when a user doesn't call Register and uses Open directly?

nice catch! adding it 👍

alexandre-normand · 2023-11-02T16:34:34Z

contrib/database/sql/sql.go

+	fullModeSupported := func() bool {
+		unsupportedDrivers := []string{"sqlserver", "oracle"}
+		for _, dr := range unsupportedDrivers {
+			if dr == driverName {


Obviously take my perspective with a grain of salt since I'm not an owner/maintainer for the go tracer but I agree that the driverName based logic is somewhat brittle. I know it's done in other places but I wonder if we could still consider doing type checking on the driver.Driver or driver.Connector.

Something like

if connector.Driver().(go_ora.OracleDriver) { // we know this is one of the oracle drivers }

github-actions · 2023-11-23T01:45:15Z

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

alexandre-normand · 2023-11-29T17:29:28Z

contrib/database/sql/option.go

+	}
+}
+
+func dbmFullModeUnsupported(driverName string, driver driver.Driver, dsn string) (string, bool) {


I like this way more robust implementation. Nice work gathering the packages/prefixes needed to do the detection.

alexandre-normand · 2023-11-29T17:29:58Z

contrib/database/sql/propagation_test.go

@@ -288,6 +288,33 @@ func TestDBMTraceContextTagging(t *testing.T) {
 	}
 }

+func TestDBMPropagation_PreventFullMode(t *testing.T) {


vandonr

nice job getting all the values those variables can take 💦

github-actions bot reviewed Nov 2, 2023

View reviewed changes

contrib/database/sql/sql.go Outdated Show resolved Hide resolved

rarguelloF changed the title ~~database/sql: prevent DBM propagation full mode with incompatible dri…~~ database/sql: prevent DBM propagation full mode with incompatible drivers Nov 2, 2023

database/sql: prevent DBM propagation full mode with incompatible dri…

b8a0110

…vers

rarguelloF force-pushed the rarguelloF/dbm-prevent-full-mode branch from 706d891 to b8a0110 Compare November 2, 2023 13:28

rarguelloF added 2 commits November 2, 2023 14:28

Merge branch 'main' into rarguelloF/dbm-prevent-full-mode

0fb5df6

refactor

02ae892

`

vandonr reviewed Nov 2, 2023

View reviewed changes

fix test

2154437

alexandre-normand reviewed Nov 2, 2023

View reviewed changes

github-actions bot added the stale Stuck for more than 1 month label Nov 23, 2023

rarguelloF removed the stale Stuck for more than 1 month label Nov 23, 2023

rarguelloF added 3 commits November 29, 2023 11:17

Merge branch 'main' into rarguelloF/dbm-prevent-full-mode

34d61b3

use different method to detect incompatible dbs

b8f8bda

Merge branch 'main' into rarguelloF/dbm-prevent-full-mode

b0096c4

rarguelloF marked this pull request as ready for review November 29, 2023 17:18

rarguelloF requested review from a team, alexandre-normand and vandonr November 29, 2023 17:18

alexandre-normand approved these changes Nov 29, 2023

View reviewed changes

vandonr approved these changes Nov 30, 2023

View reviewed changes

Merge branch 'main' into rarguelloF/dbm-prevent-full-mode

4c9ef99

rarguelloF changed the title ~~database/sql: prevent DBM propagation full mode with incompatible drivers~~ contrib/database/sql: prevent DBM propagation full mode with incompatible dbs Dec 1, 2023

github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Dec 1, 2023

ahmed-mez approved these changes Dec 12, 2023

View reviewed changes

rarguelloF enabled auto-merge (squash) December 12, 2023 13:01

Merge branch 'main' into rarguelloF/dbm-prevent-full-mode

e13e2c8

rarguelloF merged commit fa3ec4f into main Dec 12, 2023
45 of 49 checks passed

rarguelloF deleted the rarguelloF/dbm-prevent-full-mode branch December 12, 2023 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/database/sql: prevent DBM propagation full mode with incompatible dbs #2328

contrib/database/sql: prevent DBM propagation full mode with incompatible dbs #2328

rarguelloF commented Nov 2, 2023 •

edited

pr-commenter bot commented Nov 2, 2023 •

edited

vandonr Nov 2, 2023

alexandre-normand Nov 2, 2023

rarguelloF Nov 29, 2023 •

edited

alexandre-normand Nov 2, 2023

alexandre-normand Nov 2, 2023

rarguelloF Nov 29, 2023

alexandre-normand Nov 2, 2023

rarguelloF Nov 29, 2023

alexandre-normand Nov 29, 2023

alexandre-normand Nov 2, 2023

github-actions bot commented Nov 23, 2023

alexandre-normand Nov 29, 2023

alexandre-normand Nov 29, 2023

vandonr left a comment

contrib/database/sql: prevent DBM propagation full mode with incompatible dbs #2328

contrib/database/sql: prevent DBM propagation full mode with incompatible dbs #2328

Conversation

rarguelloF commented Nov 2, 2023 • edited

What does this PR do?

Motivation

Reviewer's Checklist

pr-commenter bot commented Nov 2, 2023 • edited

Benchmarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rarguelloF Nov 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vandonr left a comment

Choose a reason for hiding this comment

rarguelloF commented Nov 2, 2023 •

edited

pr-commenter bot commented Nov 2, 2023 •

edited

rarguelloF Nov 29, 2023 •

edited