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

feat: database/sql integration #893

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

aldy505
Copy link
Contributor

@aldy505 aldy505 commented Oct 18, 2024

Queries insights tracing for Go.

sentrysql/conn.go Outdated Show resolved Hide resolved
sentrysql/stmt.go Outdated Show resolved Hide resolved
sentrysql/stmt.go Outdated Show resolved Hide resolved
sentrysql/stmt.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 19, 2024

Codecov Report

Attention: Patch coverage is 70.71823% with 106 lines in your changes missing coverage. Please review.

Project coverage is 82.25%. Comparing base (ec151c7) to head (38ba84c).

Files with missing lines Patch % Lines
sentrysql/conn.go 65.00% 54 Missing and 2 partials ⚠️
sentrysql/stmt.go 68.26% 30 Missing and 3 partials ⚠️
sentrysql/driver.go 63.15% 13 Missing and 1 partial ⚠️
sentrysql/sentrysql.go 90.90% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
- Coverage   83.09%   82.25%   -0.84%     
==========================================
  Files          55       62       +7     
  Lines        5459     5821     +362     
==========================================
+ Hits         4536     4788     +252     
- Misses        767      869     +102     
- Partials      156      164       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aldy505
Copy link
Contributor Author

aldy505 commented Oct 19, 2024

I really don't like golangci-lint, it's so pedantic :(

@aldy505
Copy link
Contributor Author

aldy505 commented Oct 19, 2024

Should be good by now.

@aldy505 aldy505 marked this pull request as ready for review October 19, 2024 05:20
@aldy505 aldy505 requested a review from ribice October 19, 2024 05:20
sentrysql/operation.go Outdated Show resolved Hide resolved
@ribice
Copy link
Collaborator

ribice commented Oct 22, 2024

I'll test this locally myself this week and report back. We should also update the docs and main repository with this integration.

@ribice
Copy link
Collaborator

ribice commented Oct 25, 2024

You're missing examples in _examples. These should also include example on how to set the DSN.

@aldy505
Copy link
Contributor Author

aldy505 commented Oct 25, 2024

You're missing examples in _examples. These should also include example on how to set the DSN.

@ribice I've been occupied so much with work this week. Will find the time to work on this later.

@aldy505 aldy505 requested a review from ribice October 26, 2024 12:57
@ribice
Copy link
Collaborator

ribice commented Oct 27, 2024

LGTM Overall. Still misses some tests for the coverage, entry in CHANGELOG.MD and documentation in main repo and sentry-docs.

@aldy505
Copy link
Contributor Author

aldy505 commented Oct 27, 2024

LGTM Overall. Still misses some tests for the coverage

To be honest, I don't know how to make some of that as covered other than using old Go versions. But I might try creating another database driver implementation that does not implement XXXContext method.

...documentation in main repo and sentry-docs.

I might be able to help with the sentry-docs one, since I also need to update some of the self-hosted docs.

@cleptric
Copy link
Member

I haven't reviewed this PR in full yet, but some things I would like to be changed:

  • I would suggest we start with https://pkg.go.dev/database only, I'm not keen to add support for 3rd party libraries right away.
  • If we end up adding 3rd party dependencies, we should publish this as it's own package. See https://github.com/getsentry/sentry-go/tree/master/otel for prior art.
  • We also support Mongo DB in our Insights query module, so the naming of the package could be more generic, such as sentrydatabase
  • nolint is an immediate code smell.

@aldy505
Copy link
Contributor Author

aldy505 commented Nov 2, 2024

We are not supporting 3rd party libraries here. We only import the database driver since Go does not bring built-in database drivers and rely on third party drivers. The database drivers we're using should not going to be compiled to the resulting binary since it's only used in tests.

Although it seems like we're using it as a library here, it is not, it's still considered a database driver:

// Create a database connection for read database.
connector, err := pq.NewConnector("postgres://postgres:[email protected]:5432/postgres")
if err != nil {
fmt.Printf("failed to create a postgres connector: %s\n", err.Error())
return
}

  • We also support Mongo DB in our Insights query module, so the naming of the package could be more generic, such as sentrydatabase

MongoDB does not have and does not conform to the database/sql driver. So we're not going to add it into the enum here. Instrumentation for MongoDB should be specific for the Go MongoDB library.

  • nolint is an immediate code smell.

Since we're wrapping a database driver and thus making us another database driver, we are required to implement multiple methods that are considered duplicates by the linter, yet again, from the driver documentation, we're still required to implement those.

Comment on lines +36 to +48
span := parentSpan.StartChild("db.sql.exec", sentry.WithDescription(s.query))
s.config.SetData(span, s.query)
defer span.Finish()

result, err := s.originalStmt.Exec(args) //nolint:staticcheck // We must support legacy clients
if err != nil {
span.Status = sentry.SpanStatusInternalError
return nil, err
}

span.Status = sentry.SpanStatusOK

return result, nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section is not covered by tests, yet I don't know how to make it so.

I've added this check beforehand:

if s.ctx == nil && ctx != nil {
  s.ctx = ctx
}

But it doesn't seem to help anyway.

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.

3 participants