Capture Cockroach DB config in sentry-rails ActiveRecordSubscriber

[natikgadzhi]

Summary

This pull request allows sentry-ruby to grab DB configuration from connection.instance_variable_get(:@config) if that's defined and all else failed, but continue gracefully if the config is not there.

Closes #2110.

Changes

I've set up a test Rails app with Cockroach DB adapter to see what's inside of it's connection. The problem is, connection.pool is NullConnectionPool, but the connection itself has the config variable on it. So I'm grabbing data from this.

The change itself is non-destructive and safe to merge (i.e, it checks for whether the variable is there, and it only grabs data from it if all other sources fail). But, it does not have a unit test specific to Cockroach DB adapter.

I've verified that this code does capture DB config data and presents it in the db.sql.active_record Span in the Web UI, though.

TODO

• I could add a rather naive unit test that tests that if we're capturing data from connection.instance_variable_get(:@config), the event still gets recorded with the right data.

Should I, or good to go as is? /cc @sl0thentr0py and @st0012

[codecov[bot]]

Codecov Report

Merging #2182 (85f1ccf) into master (4c8110c) will increase coverage by 0.02%. The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2182      +/-   ##
==========================================
+ Coverage   97.31%   97.34%   +0.02%     
==========================================
  Files          99       99              
  Lines        3691     3693       +2     
==========================================
+ Hits         3592     3595       +3     
+ Misses         99       98       -1     

Components	Coverage Δ
sentry-ruby	98.03% <ø> (ø)
sentry-rails	95.18% <100.00%> (+0.20%)	:arrow_up:
sentry-sidekiq	94.50% <ø> (ø)
sentry-resque	92.06% <ø> (ø)
sentry-delayed_job	94.44% <ø> (ø)
sentry-opentelemetry	100.00% <ø> (ø)

Files	Coverage Δ
...b/sentry/rails/tracing/active_record_subscriber.rb	87.09% <100.00%> (+4.33%)	:arrow_up:

[natikgadzhi]

• Updated the changelog entry
• Added a unit test

[natikgadzhi]

We should bring in the adapter gem as a development dependency and write an isolated test just for it.

@st0012, agreed, if we choose to go with the path of this pull request and make the failsafe config reader, I can absolutely bring in the CockroachDB adapter as a dependency and test it in an isolated way. I think that'll also give us a red flag when they patch things up on their side.

Or, the adapter doesn't follow other adapters' convention to properly set things up.

Perhaps that's the root cause, and you're right that fixing it upstream is better than patching the SDK. I think @sl0thentr0py is hinting at that, too?

I can start with adding a clear test case which works directly with their adapter to highlight the problem. Then, I can start an issue in their repo and see if I can fix it upstream.

Since Sentry SDK is not crashing, just not reporting a few strings, than this PR is not urgently needed, too.

Let's keep this open so I can put the test in this PR branch, but then let's see how we feel about merging this as a stopgap, or just waiting for the upstream fix. No hard feelings either way!