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

contrib/database/sql: trace connection time #1154

Merged
merged 8 commits into from Feb 4, 2022

Conversation

ajgajg1134
Copy link
Contributor

This builds on PR #794

Fixed #760

@ajgajg1134 ajgajg1134 added this to the 1.37.0 milestone Jan 31, 2022
@ajgajg1134 ajgajg1134 self-assigned this Jan 31, 2022
@ajgajg1134 ajgajg1134 requested a review from a team January 31, 2022 20:39
@gbbr gbbr changed the title Feature/contrib sql conn trace contrib/database/sql: trace connection time Feb 1, 2022
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

This looks good to me! I just have a few questions.

contrib/database/sql/sql_test.go Outdated Show resolved Hide resolved
contrib/database/sql/sql_test.go Outdated Show resolved Hide resolved
contrib/internal/sqltest/sqltest.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

A few more questions, but looks good. I'd also recommend you try this out and run it against the actual Datadog UI/app and make sure that what shows up there is what's expected.

contrib/database/sql/conn_test.go Show resolved Hide resolved
contrib/internal/sqltest/sqltest.go Show resolved Hide resolved
contrib/internal/sqltest/sqltest.go Show resolved Hide resolved
@ajgajg1134
Copy link
Contributor Author

A few more questions, but looks good. I'd also recommend you try this out and run it against the actual Datadog UI/app and make sure that what shows up there is what's expected.

Yep! I ran this against a local postgres instance with the datadog agent and verified that the connect spans showed in the Datadog Trace UI as desired. Not sure if there's any other additional manual or integration testing that should be done?

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Really nice work Andrew! Thanks for doing this. 🚀

@ajgajg1134 ajgajg1134 merged commit d1b2aba into DataDog:v1 Feb 4, 2022
@ajgajg1134 ajgajg1134 deleted the feature/contrib-sql-conn-trace branch February 4, 2022 15:32
@felixge
Copy link
Member

felixge commented Feb 10, 2022

Not sure if there's any other additional manual or integration testing that should be done?

We can also test this in our internal reliability env. I'll ping you in slack about it.

@felixge
Copy link
Member

felixge commented Dec 8, 2022

@ajgajg1134 sorry for the random ping on this: Do you remember if this also traces time spent while waiting on an already established connection from the pool (b/c the max connection limit has been reached)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proposal: database/sql: should trace also the time spent connecting
3 participants