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]

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%)	⬆️
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%)	⬆️

[natikgadzhi]

• Updated the changelog entry
• Added a unit test

[sl0thentr0py]

can you point in the rails/cockroach adapter source where this :config var is defined?

[natikgadzhi]

I have a sneaky suspicion that @config on the connection is available in any connection, not just CockroachDB: https://github.com/rails/rails/blob/9c22f35440ab85718ebf48e26b8944032c737193/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb#L135C36-L135C36

That's what makes this code more or less sane to me — if all else fails, the config should be a hash on the connection object, whatever it is.

[sl0thentr0py]

yea but that's an instance variable, not part of the official class api. There was a reason I used the pool/db_config when I implemented this because that's the standard way in rails now.

[st0012]

ConnectionPool should be assigned automatically during the connection establishing process, e.g.

https://github.com/rails/rails/blob/9c22f35440ab85718ebf48e26b8944032c737193/activerecord/lib/active_record/connection_adapters/pool_config.rb#L70

The fact that for Cockroach DB it remains NullPool makes me think that it may not exactly follow Rails' convention to set up its adapter. To be clear, I didn't find the code that directly contribute to this result so I may be wrong. But IMO it's the most likely case.

If that's the case, then it's the adapter gem that needs to be fixed, not the SDK.

[natikgadzhi]

I think you're right, and perhaps adding janky patches instead of fixing the adapter itself is not the way 👀

But, I'm not deeply experienced enough, maybe I should go debug how cockroachDB adapter instanciates the connection some more.

[sl0thentr0py]

I also think that NullPool here is a code smell, especially since the cockroach adapter is deriving from the postgres adapter, I'll read the source tomorrow and try to figure out if a patch upstream is better.

[natikgadzhi]

I only spent a couple hours on this, and I'm not all that experienced in how ActiveRecod is structured, so chances are, I missed the root cause of this. Agreed re: smell.

[st0012]

IMO, the fact that we need to artificially stub pool here means:

• We should bring in the adapter gem as a development dependency and write an isolated test just for it.
• Or, the adapter doesn't follow other adapters' convention to properly set things up.

[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!