Capture Cockroach DB config in sentry-rails ActiveRecordSubscriber #2182
Conversation
sentry-rails/lib/sentry/rails/tracing/active_record_subscriber.rb
Outdated
Show resolved
Hide resolved
Codecov Report
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
|
natikgadzhi
force-pushed
the
ng/bugfix/sentry-rails/cockroach-db-activerecord-subscriber
branch
from
86a0ffc
to
dbccd78
Compare
natikgadzhi
force-pushed
the
ng/bugfix/sentry-rails/cockroach-db-activerecord-subscriber
branch
from
dbccd78
to
d67c265
Compare
|
| @@ -31,6 +31,10 @@ def self.subscribe! | |||
| connection.pool.db_config.configuration_hash | |||
| elsif connection.pool.respond_to?(:spec) | |||
| connection.pool.spec.config | |||
| # CockroachDB pool shows up as NullPool, but we can grab | |||
| # it's configuration from the instance variable. | |||
| elsif connection.instance_variable_defined?(:@config) | |||
There was a problem hiding this comment.
can you point in the rails/cockroach adapter source where this :config var is defined?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -31,6 +31,10 @@ def self.subscribe! | |||
| connection.pool.db_config.configuration_hash | |||
| elsif connection.pool.respond_to?(:spec) | |||
| connection.pool.spec.config | |||
| # CockroachDB pool shows up as NullPool, but we can grab | |||
There was a problem hiding this comment.
ConnectionPool should be assigned automatically during the connection establishing process, e.g.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| # so we have to grab the configuration off the Connection itself. | ||
| # See https://github.com/getsentry/sentry-ruby/issues/2110 | ||
| config = { adapter: "sqlite3", database: "dummy-database" } | ||
| allow_any_instance_of(ActiveRecord::ConnectionAdapters::SQLite3Adapter).to receive(:pool).and_return(nil) |
There was a problem hiding this comment.
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.
@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.
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! |
Summary
This pull request allows
sentry-rubyto grab DB configuration fromconnection.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.poolisNullConnectionPool, 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_recordSpan in the Web UI, though.TODO
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