Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Bug Fixes

- Network errors raised in `Sentry::HTTPTransport` will no longer be reported to Sentry [#2178](https://github.com/getsentry/sentry-ruby/pull/2178)
- `sentry-rails` will now capture Cockroach DB adapter config into spans data [#2182](https://github.com/getsentry/sentry-ruby/pull/2182)

## 5.14.0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

@st0012 st0012 Nov 29, 2023

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

# it's configuration from the instance variable.
elsif connection.instance_variable_defined?(:@config)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Expand Down