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.

Sign up for GitHub

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

Support sending custom tags when using pollDBStats #3067

Closed
wants to merge 2 commits into from
Closed

Support sending custom tags when using pollDBStats #3067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
011294d
Support sending custom tags when using pollDBStats
Jan 6, 2025
0424082
gofmt
Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/apps/latest_major_version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ package main
import (
"encoding/json"
"fmt"
"sort"
"github.com/Masterminds/semver/v3"
"golang.org/x/mod/modfile"
"net/http"
"os"
"regexp"
"sort"
"strings"
)

Expand Down
20 changes: 10 additions & 10 deletions contrib/database/sql/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,20 @@ var interval = 10 * time.Second

// pollDBStats calls (*DB).Stats on the db at a predetermined interval. It pushes the DBStats off to the statsd client.
// the caller should always ensure that db & statsd are non-nil
func pollDBStats(statsd internal.StatsdClient, db *sql.DB) {
func pollDBStats(statsd internal.StatsdClient, db *sql.DB, tags []string) {
log.Debug("DB stats will be gathered and sent every %v.", interval)
for range time.NewTicker(interval).C {
log.Debug("Reporting DB.Stats metrics...")
stat := db.Stats()
statsd.Gauge(MaxOpenConnections, float64(stat.MaxOpenConnections), []string{}, 1)
statsd.Gauge(OpenConnections, float64(stat.OpenConnections), []string{}, 1)
statsd.Gauge(InUse, float64(stat.InUse), []string{}, 1)
statsd.Gauge(Idle, float64(stat.Idle), []string{}, 1)
statsd.Gauge(WaitCount, float64(stat.WaitCount), []string{}, 1)
statsd.Timing(WaitDuration, stat.WaitDuration, []string{}, 1)
statsd.Gauge(MaxIdleClosed, float64(stat.MaxIdleClosed), []string{}, 1)
statsd.Gauge(MaxIdleTimeClosed, float64(stat.MaxIdleTimeClosed), []string{}, 1)
statsd.Gauge(MaxLifetimeClosed, float64(stat.MaxLifetimeClosed), []string{}, 1)
statsd.Gauge(MaxOpenConnections, float64(stat.MaxOpenConnections), tags, 1)
statsd.Gauge(OpenConnections, float64(stat.OpenConnections), tags, 1)
statsd.Gauge(InUse, float64(stat.InUse), tags, 1)
statsd.Gauge(Idle, float64(stat.Idle), tags, 1)
statsd.Gauge(WaitCount, float64(stat.WaitCount), tags, 1)
statsd.Timing(WaitDuration, stat.WaitDuration, tags, 1)
statsd.Gauge(MaxIdleClosed, float64(stat.MaxIdleClosed), tags, 1)
statsd.Gauge(MaxIdleTimeClosed, float64(stat.MaxIdleTimeClosed), tags, 1)
statsd.Gauge(MaxLifetimeClosed, float64(stat.MaxLifetimeClosed), tags, 1)
Comment on lines +41 to +49
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtoffl01 Was there any reason behind not allowing tags from the beginning?

Checking the code, I see that for the purpose of this PR, it should be enough to set DD_TAGS or OTEL_PROPAGATORS (if using OTel) to set these global tags.

WDYT @KaibutsuX?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have enough context or documentation to know what DD_TAGS is or how I would set it as an end-user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DD_TAGS is properly explained in our Go tracer public documentation.

We understand this should be enough to achieve what you are trying with this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That links says: "A list of default tags to be added to every span and profile." From my tracing through the code, I could not see that this same list of user-supplied tags was included in these metrics used in pollDBStats

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we understand it may not that be clear from the code. Sorry about that. It inherits the global tags from the tracer config, set by globalconfig.SetStatsTags, which is initialized by newConfig, and retrieved when setting up the DB statsd client.

We'll update the public documentation to make sure it reflects this detail.

}
}

Expand Down
8 changes: 8 additions & 0 deletions contrib/database/sql/option.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type config struct {
tags map[string]interface{}
dbmPropagationMode tracer.DBMPropagationMode
dbStats bool
statsTags []string
statsdClient internal.StatsdClient
}

Expand Down Expand Up @@ -292,3 +293,10 @@ func WithDBStats() Option {
cfg.dbStats = true
}
}

func WithDBStatsTags(tags []string) Option {
Comment on lines +296 to +297
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func WithDBStatsTags(tags []string) Option {
// WithDBStatsTags enables DBStats metrics collection and sets the given tags for those metrics.
func WithDBStatsTags(tags map[string]string) Option {

Could we make this a map[string]string? Since this is how we model tags generally everywhere else.
We can keep cfg.statsTags as a []string and do the transformation here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dd-trace-go/contrib/database/sql/metrics.go

Line 60 in 137578f

func statsTags(c *config) []string {

dd-trace-go/internal/statsd.go

Line 16 in c345adb

type StatsdClient interface {

That does make more sense, but since your public interfaces accept []string with an only an expected convention of using : as the delimiter between keys/values, I think it's probably better to address those public places first and/or update documentation.

I'm closing this as I no longer have the time to spend on it.

return func(cfg *config) {
cfg.dbStats = true
cfg.statsTags = tags
}
}
2 changes: 1 addition & 1 deletion contrib/database/sql/sql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ func OpenDB(c driver.Connector, opts ...Option) *sql.DB {
}
db := sql.OpenDB(tc)
if cfg.dbStats && cfg.statsdClient != nil {
go pollDBStats(cfg.statsdClient, db)
go pollDBStats(cfg.statsdClient, db, cfg.statsTags)
}
return db
}
Expand Down