Add 'rails.db.name' tag to ActiveRecord #270
Conversation
test/contrib/rails/database_test.rb
Outdated
| @@ -1,4 +1,5 @@ | |||
| require 'helper' | |||
| require 'pry' | |||
There was a problem hiding this comment.
I think we should remove it, right?
There was a problem hiding this comment.
No worries, reviews are meant for that 💯
delner
force-pushed
the
add_active_record_database_name_tag
branch
from
4e27f69
to
2656719
Compare
lib/ddtrace/contrib/rails/utils.rb
Outdated
| @@ -50,6 +50,10 @@ def self.adapter_name | |||
| normalize_vendor(connection_adapter) | |||
| end | |||
|
|
|||
| def self.database_name | |||
There was a problem hiding this comment.
considering this method is just an alias, is there any reason for this additional level of indirection?
There was a problem hiding this comment.
I don't think there's much reason beyond just having naming and structure consistent with adapter_name. We could reduce this method to an alias or simply remove it altogether.
| @@ -21,6 +21,7 @@ def self.sql(_name, start, finish, _id, payload) | |||
| tracer = Datadog.configuration[:rails][:tracer] | |||
| database_service = Datadog.configuration[:rails][:database_service] | |||
| adapter_name = Datadog::Contrib::Rails::Utils.adapter_name | |||
| database_name = Datadog::Contrib::Rails::Utils.database_name | |||
delner
force-pushed
the
add_active_record_database_name_tag
branch
from
2656719
to
6bf2b8a
Compare
|
Hello @senny ! I see, let us investigate on it since the meaning of the tag was just to provide the database name and not exactly the database connection. This should be implemented someway, since you may want to separate properly (for instance) replicas queries from the main database connection. Is that correct? |
|
@palazzem yes, ideally we would get the connection name as a tag. In our case though, we are actually using different databases with the same Rails application. What we would need is a way to separate queries that are going to e.g. a reporting or a archive database and not the primary. Since they all have a different database-name, it would have been sufficient for me to implement the split. We do use follower databases as well where obviously the database name would not be sufficient. In that case we would need the connection name to tell them apart. |
This pull request adds the database name as a tag (
rails.db.name) to ActiveRecord traces.