sentry-go icon indicating copy to clipboard operation
sentry-go copied to clipboard

feat: database/sql integration

Open aldy505 opened this issue 1 year ago • 11 comments

Queries insights tracing for Go.

aldy505 avatar Oct 18 '24 10:10 aldy505

Codecov Report

:x: Patch coverage is 83.64780% with 52 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 85.92%. Comparing base (c243873) to head (1a78d47).

Files with missing lines Patch % Lines
sentrysql/conn.go 82.43% 20 Missing and 6 partials :warning:
sentrysql/stmt.go 85.86% 8 Missing and 5 partials :warning:
sentrysql/driver.go 65.62% 10 Missing and 1 partial :warning:
sentrysql/sentrysql.go 92.00% 1 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
- Coverage   86.07%   85.92%   -0.16%     
==========================================
  Files          62       69       +7     
  Lines        6090     6408     +318     
==========================================
+ Hits         5242     5506     +264     
- Misses        634      674      +40     
- Partials      214      228      +14     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Oct 19 '24 02:10 codecov[bot]

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

aldy505 avatar Oct 19 '24 02:10 aldy505

Should be good by now.

aldy505 avatar Oct 19 '24 05:10 aldy505

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

ribice avatar Oct 22 '24 10:10 ribice

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

ribice avatar Oct 25 '24 11:10 ribice

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 avatar Oct 25 '24 12:10 aldy505

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

ribice avatar Oct 27 '24 11:10 ribice

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.

aldy505 avatar Oct 27 '24 12:10 aldy505

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.

cleptric avatar Oct 28 '24 08:10 cleptric

  • 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 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: https://github.com/getsentry/sentry-go/blob/c5fa49de994f32da77499c632e8e512724556a95/_examples/sql/main.go#L40-L45

  • 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.

aldy505 avatar Nov 02 '24 12:11 aldy505

Don't let this die! :) I'm sure many of us in the community will be grateful for an officially supported DB integration

jazzido avatar Nov 28 '24 18:11 jazzido

@cleptric @giortzisg any updates?

aldy505 avatar Sep 25 '25 07:09 aldy505

We're currently doing a big refactoring on the transport layer, will have a look after that.

giortzisg avatar Sep 25 '25 07:09 giortzisg