sentry-go
sentry-go copied to clipboard
feat: database/sql integration
Queries insights tracing for Go.
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).
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.
I really don't like golangci-lint, it's so pedantic :(
Should be good by now.
I'll test this locally myself this week and report back. We should also update the docs and main repository with this integration.
You're missing examples in _examples. These should also include example on how to set the DSN.
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.
LGTM Overall. Still misses some tests for the coverage, entry in CHANGELOG.MD and documentation in main repo and sentry-docs.
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.
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 nolintis an immediate code smell.
- 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.
nolintis 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.
Don't let this die! :) I'm sure many of us in the community will be grateful for an officially supported DB integration
@cleptric @giortzisg any updates?
We're currently doing a big refactoring on the transport layer, will have a look after that.