contrib/database/sql: Close DB Stats goroutine on db.Close()#3025
Conversation
github-actions
Bot
added
the
apm:ecosystem
Datadog ReportBranch report: ❌ 1 Failed (0 Known Flaky), 5014 Passed, 71 Skipped, 2m 44.91s Total Time ❌ Failed Tests (1)
|
0692055 to
37b52f9
Compare
darccio
left a comment
darccio
left a comment
There was a problem hiding this comment.
LGTM. I left some small nits.
| }) | ||
| } | ||
|
|
||
| func GetStopChan() chan struct{} { |
There was a problem hiding this comment.
| func GetStopChan() chan struct{} { | |
| func StopChan() chan struct{} { |
This would be a better name.
| mu sync.Mutex | ||
| ) | ||
|
|
||
| func InitStopChan() { |
There was a problem hiding this comment.
| func InitStopChan() { | |
| func Init() { |
Just in case this package evolves and needs to initialize more things beyond the stop chan.
Datadog ReportBranch report: ✅ 0 Failed, 5222 Passed, 72 Skipped, 1m 53.27s Total Time |
mtoffl01
changed the title
kakkoyun
left a comment
kakkoyun
left a comment
There was a problem hiding this comment.
Looking good 👍 I have a question though.
|
|
||
| // Close closes the dbClose channel | ||
| // This method will be invoked when DB.Close() is called, which we expect to occur only once: https://cs.opensource.google/go/go/+/refs/tags/go1.23.4:src/database/sql/sql.go;l=918-950 | ||
| func (t *tracedConnector) Close() error { |
There was a problem hiding this comment.
This makes me realize we probably had a bug before, since this wraps the original driver.Connector, if the original one implemented Close() error, our traced version didn't (until this PR).
Maybe we should internally call the original connector Close method, in case it implements io.Closer?
(btw this is ok to do outside of this PR, since this issue has been there before).
4 participants
What does this PR do?
Introduces a stop condition for the
pollDBStatsgoroutine whendb.Close()has been invoked by the user.Motivation
Protecting against runaway goroutines.
Details
Background
To register a sql driver for instrumentation, a user must call sqltrace.Open, which passes our
tracedConnectorinto the OpenDB function. ThistracedConnectoris used as thec driver.Connectorargument to database/sql's OpenDB function, which is set on the DB.connector field.When
db.Close()is called by the user, the database/sql code checks whether theDB.connectorsatisfies theio.Closerinterface and, if so, callsconnector.Close().Therefore, to identify when a user has called
db.Close(), we can implement (and control) atracedConnector.Close()method.Implementation
A new
tracedConnector.dbClosechannel is closed whentracedConnector.Close()is invoked. ThepollDBStatsgoroutine returns (stops) when there is activity on thedbClosechannel.In this way, we've tied the
pollDBStatsgoroutine to the lifecycle of the db.Next Steps
Once this approach is approved, we will do the same thing for the contrib/jackc/pgx
pollPoolStatsgoroutine.Reviewer's Checklist
v2-devbranch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!