contrib/database/sql: Submit DBStats as Datadog metrics #2543

mtoffl01 · 2024-02-07T16:47:19Z

What does this PR do?

Publicly, this PR exposes a contrib/database/sql option to enable polling and submission of DBStats as metrics to Datadog
Ref: https://pkg.go.dev/database/sql#DBStats
This PR also lays the groundwork on the tracer package for submitting statsd payloads from other contrib integrations in the future, if desired

Motivation

Customer requests
#2303

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
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.
Add an appropriate team label so this PR gets put in the right place for the release notes.
Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

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!

…ib, and expose tracer statsd client

contrib/database/sql/option.go

contrib/database/sql/sql.go

ddtrace/tracer/metrics.go

contrib/database/sql/sql.go

pr-commenter · 2024-02-07T17:11:49Z

Benchmarks

Benchmark execution time: 2024-02-27 18:57:47

Comparing candidate commit 9523300 in PR branch mtoff/sql_metrics with baseline commit b855917 in branch main.

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

scenario:BenchmarkPartialFlushing/Disabled-24

🟩 avgHeapInUse(Mb) [-6.609MB; -1.303MB] or [-10.273%; -2.025%]

…nd tracer to share stats from former to latter

internal/globalconfig/globalconfig.go

internal/statsd.go

ddtrace/tracer/metrics.go

ddtrace/tracer/metrics_test.go

…st 1 arg

…trib and tracer

internal/statsd.go

katiehockman

Overall this looks like a sound approach! I'll read over the design doc when you send it, and that'll probably help clarify things too. Nice work! 🥳

contrib/database/sql/sql.go

contrib/database/sql/option.go

ddtrace/tracer/option.go

internal/statsd.go

Co-authored-by: Katie Hockman <[email protected]>

… types

internal/statsd.go

internal/statsd_test.go

ddtrace/tracer/metrics_test.go

…file

…en passed to Register

contrib/database/sql/example_test.go

…tscarrier

internal/globalconfig/globalconfig_test.go

contrib/database/sql/option.go

katiehockman

Just a few small comments, but otherwise looks good! We should be good to merge after this.

contrib/database/sql/sql_test.go

internal/globalconfig/globalconfig_test.go

ddtrace/tracer/option.go

contrib/database/sql/sql_test.go

katiehockman

Since these are just test-related comments, I'll go ahead and approve and we can merge after those are addressed 😄

…to mtoff/sql_metrics

felixge · 2024-03-02T14:43:58Z

contrib/database/sql/metrics.go

+	MaxIdleClosed      = tracerPrefix + "sql.db.connections.closed.max_idle_conns"
+	MaxIdleTimeClosed  = tracerPrefix + "sql.db.connections.closed.max_idle_time"
+	MaxLifetimeClosed  = tracerPrefix + "sql.db.connections.closed.max_lifetime"
+)


NIT: Should these be public? (if not, we could still make them private before the next release starts on Monday, cc @dianashevchenko )

No, there's no need for these to be public, that was just an oversight!

Implementation 1: Run goroutine for polling metrics from within contr…

6eafc80

…ib, and expose tracer statsd client

mtoffl01 requested a review from a team as a code owner February 7, 2024 16:47

github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Feb 7, 2024

github-actions bot reviewed Feb 7, 2024

View reviewed changes

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

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

ddtrace/tracer/metrics.go Outdated Show resolved Hide resolved

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

nits: Added public fn definition and cleaned up if/else statement

8ed75ef

mtoffl01 marked this pull request as draft February 7, 2024 17:54

database/sql & tracer: implemented channel of comms between contrib a…

11cd3a1

…nd tracer to share stats from former to latter

github-actions bot reviewed Feb 8, 2024

View reviewed changes

internal/globalconfig/globalconfig.go Outdated Show resolved Hide resolved

internal/statsd.go Outdated Show resolved Hide resolved

ddtrace/tracer/metrics.go Outdated Show resolved Hide resolved

ddtrace/tracer/metrics.go Outdated Show resolved Hide resolved

nits from github bot

f92d158

github-actions bot reviewed Feb 8, 2024

View reviewed changes

mtoffl01 added 4 commits February 8, 2024 13:41

foundation for sql_test tests

9bb05ba

update other calls to reportContribMetrics now that the fn accepts ju…

df2f0b2

…st 1 arg

Implementing new type StatsCarrier to share statsd client between con…

4f64c73

…trib and tracer

fix panic on attempt to close a closed channel on statscarrier

d1a72bf

github-actions bot reviewed Feb 12, 2024

View reviewed changes

internal/statsd.go Outdated Show resolved Hide resolved

removed superfluous log test

57b83b8

katiehockman reviewed Feb 12, 2024

View reviewed changes

Update contrib/database/sql/sql.go

0296193

Co-authored-by: Katie Hockman <[email protected]>

mtoffl01 mentioned this pull request Feb 15, 2024

statsdtest: Move mock statsd client for testing into its own package #2564

Merged

8 tasks

mtoffl01 added 2 commits February 15, 2024 15:21

Fleshing out tests and making changes as a result of tests

38b969b

Changed Stat implementation to interface, with gauge count and timing…

321e2a7

… types

github-actions bot reviewed Feb 15, 2024

View reviewed changes

internal/statsd.go Outdated Show resolved Hide resolved

internal/statsd_test.go Outdated Show resolved Hide resolved

ddtrace/tracer/metrics_test.go Outdated Show resolved Hide resolved

mtoffl01 added 7 commits February 16, 2024 10:47

go fmt'd

015bcfc

Merge branch 'main' into mtoff/sql_metrics

8d3a473

Moved metrics collection out of sql.go file and into its own metrics …

1b1a612

…file

Remove TestContribStats from metrics_test.go

ad442c4

added lock to globalconfig's statscarrier to protect against data race

041f64d

fix locks on globalconfig & change pollDBStats function order

55501ea

Added new TestDBStats fn

8ae57ad

Expanded godoc comments, examples and fixed bug in dbstats feature wh…

58cedac

…en passed to Register

github-actions bot reviewed Feb 22, 2024

View reviewed changes

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

mtoffl01 added 3 commits February 22, 2024 15:40

Add last todos and tags

fc7c049

gofmt

7c800b7

Made db polling interval nonconfigurable, and fixed lock issue in sta…

098595d

…tscarrier

github-actions bot reviewed Feb 22, 2024

View reviewed changes

internal/globalconfig/globalconfig_test.go Outdated Show resolved Hide resolved

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

mtoffl01 and others added 11 commits February 22, 2024 18:09

go fmt

6541d9d

Merge branch 'main' into mtoff/sql_metrics

441d233

Fixed flaky tests

92547d3

merge with remote branch

0becfbd

Remove slices dependency

50daa34

Update example_test.go

41cac05

Update metrics.go

9a85b14

Update globalconfig.go

77cfd1e

Update statsd.go

0e8dbc2

change all stats to gauge

2e356e9

Update example_test.go

c6ed0ad

katiehockman reviewed Feb 26, 2024

View reviewed changes

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

internal/globalconfig/globalconfig_test.go Outdated Show resolved Hide resolved

ddtrace/tracer/option.go Show resolved Hide resolved

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

katiehockman approved these changes Feb 26, 2024

View reviewed changes

mtoffl01 and others added 5 commits February 26, 2024 17:32

Merge branch 'mtoff/sql_metrics' of github.com:DataDog/dd-trace-go in…

8ebc177

…to mtoff/sql_metrics

Reduce pollDBStats interval for testing purposes

b7eaf30

use t.Cleanup in globalconfig_test.go

3cdae9f

rework TestOpenOptions in sql_test.go

df8d794

Merge branch 'main' into mtoff/sql_metrics

f016742

katiehockman enabled auto-merge (squash) February 27, 2024 15:10

mtoffl01 and others added 2 commits February 27, 2024 13:42

reworked TestOpenOptions logic

9523300

Merge branch 'main' into mtoff/sql_metrics

834354b

katiehockman merged commit 8a989a5 into main Feb 27, 2024
155 of 157 checks passed

katiehockman deleted the mtoff/sql_metrics branch February 27, 2024 19:20

felixge reviewed Mar 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/database/sql: Submit DBStats as Datadog metrics #2543

contrib/database/sql: Submit DBStats as Datadog metrics #2543

mtoffl01 commented Feb 7, 2024 •

edited

Loading

pr-commenter bot commented Feb 7, 2024 •

edited

Loading

katiehockman left a comment

katiehockman left a comment

katiehockman left a comment

felixge Mar 2, 2024

mtoffl01 Mar 7, 2024

contrib/database/sql: Submit DBStats as Datadog metrics #2543

contrib/database/sql: Submit DBStats as Datadog metrics #2543

Conversation

mtoffl01 commented Feb 7, 2024 • edited Loading

What does this PR do?

Motivation

Reviewer's Checklist

pr-commenter bot commented Feb 7, 2024 • edited Loading

Benchmarks

scenario:BenchmarkPartialFlushing/Disabled-24

katiehockman left a comment

Choose a reason for hiding this comment

katiehockman left a comment

Choose a reason for hiding this comment

katiehockman left a comment

Choose a reason for hiding this comment

felixge Mar 2, 2024

Choose a reason for hiding this comment

mtoffl01 Mar 7, 2024

Choose a reason for hiding this comment

mtoffl01 commented Feb 7, 2024 •

edited

Loading

pr-commenter bot commented Feb 7, 2024 •

edited

Loading