-
-
Notifications
You must be signed in to change notification settings - Fork 505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Capture Cockroach DB config in sentry-rails ActiveRecordSubscriber #2182
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you point in the rails/cockroach adapter source where this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a sneaky suspicion that 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
connection.instance_variable_get(:@config) | ||
end | ||
|
||
next unless db_config | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,38 @@ | |
expect(data["db.name"]).to include("db") | ||
expect(data["db.system"]).to eq("sqlite3") | ||
end | ||
|
||
it "records database configuration if connection.pool is not available" do | ||
# Some adapters (like CockroachDB) don't provide ConnectionPool, | ||
# 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, the fact that we need to artificially stub
|
||
allow_any_instance_of(ActiveRecord::ConnectionAdapters::SQLite3Adapter).to receive(:instance_variable_get).with(:@config).and_return(config) | ||
|
||
transaction = Sentry::Transaction.new(sampled: true, hub: Sentry.get_current_hub) | ||
Sentry.get_current_scope.set_span(transaction) | ||
|
||
Post.all.to_a | ||
|
||
transaction.finish | ||
|
||
expect(transport.events.count).to eq(1) | ||
|
||
transaction = transport.events.first.to_hash | ||
expect(transaction[:type]).to eq("transaction") | ||
expect(transaction[:spans].count).to eq(1) | ||
|
||
span = transaction[:spans][0] | ||
expect(span[:op]).to eq("db.sql.active_record") | ||
expect(span[:description]).to eq("SELECT \"posts\".* FROM \"posts\"") | ||
expect(span[:tags].key?(:cached)).to eq(false) | ||
expect(span[:trace_id]).to eq(transaction.dig(:contexts, :trace, :trace_id)) | ||
|
||
data = span[:data] | ||
expect(data["db.name"]).to eq("dummy-database") | ||
expect(data["db.system"]).to eq("sqlite3") | ||
end | ||
end | ||
|
||
context "when transaction is not sampled" do | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.