perf: cache static span attributes in Trilogy instrumentation#2089
perf: cache static span attributes in Trilogy instrumentation#2089schlubbi wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
Cache base span attributes (db.system, net.peer.name, db.name, db.user, peer_service) at connection initialization rather than rebuilding a new hash on every query. Use Hash#dup from the frozen cached hash instead of constructing from scratch each time. Also cache database_name and database_user as instance variables, and read Trilogy.attributes once per query() call instead of twice. Benchmarked improvements: - client_attributes (no sql): 864ns → 163ns (5.3×) - client_attributes (with sql): 946ns → 235ns (4.0×) - database_name: 318ns → 58ns (5.5×) - database_user: 193ns → 59ns (3.3×) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Align hash arguments per Layout/HashAlignment - Remove trailing commas per Style/TrailingCommaInHashLiteral - Use single-quoted strings per Style/StringLiterals Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| @@ -92,11 +103,11 @@ def client_attributes(sql = nil) | |||
| end | |||
|
|
|||
| def database_name | |||
There was a problem hiding this comment.
This question applies to database_name and `database_user.
Do we need these helper methods anymore since you are referencing them using direct instance variable names elsewhere?
| attributes[::OpenTelemetry::SemanticConventions::Trace::DB_NAME] = @_otel_database_name if @_otel_database_name | ||
| attributes[::OpenTelemetry::SemanticConventions::Trace::DB_USER] = @_otel_database_user if @_otel_database_user |
There was a problem hiding this comment.
Now that I am looking at this a bit closer, why did we treat the DB name and user separately from the net.peer.name attribute before this change 🤔?
Do we use the ivars for any other reason than to populate these attributes?
| private | ||
|
|
||
| def client_attributes(sql = nil) | ||
| def populate_base_attributes |
There was a problem hiding this comment.
populate an build are only called together in the constructor right?
Later on the code only uses the ivar https://github.com/open-telemetry/opentelemetry-ruby-contrib/pull/2089/changes#diff-6ba5c9beb3eb94883042f29a069003cab7f6296395e612e75b0abe00c98abbb3R88
Can we inline this all and only keep the has around once with all of the static attributes in it?
- Inline populate_base_attributes into initialize - Merge build_base_attributes into _build_otel_base_attributes - Remove database_name and database_user accessor methods - Use local variable for database_user in builder (no ivar needed) - Keep @_otel_database_name as ivar (needed by query() for span naming) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
The Trilogy instrumentation rebuilds a hash of static span attributes on every
query()call. Attributes likedb.system,net.peer.name,db.name,db.user, andpeer_servicenever change after connection initialization, yet a new hash is allocated, populated, and then merged on every query. This PR caches these static attributes at initialization time and usesHash#dupon the hot path.Changes
1. Cache base attributes at initialization
A new
populate_base_attributesmethod runs duringinitializeand builds a frozen hash of static attributes (db.system,net.peer.name,db.name,db.user,peer_service). On eachquery()call,client_attributesnow calls@_otel_base_attributes.dupinstead of constructing a new hash from scratch.2. Cache
database_nameanddatabase_useras instance variablesPreviously,
database_nameanddatabase_userperformed a hash lookup onconnection_optionson every call. These values are now cached as@_otel_database_nameand@_otel_database_userduring initialization.3. Single
Trilogy.attributeslookup per queryThe
query()method previously calledOpenTelemetry::Instrumentation::Trilogy.attributestwice — once to extractDB_OPERATIONfor span naming, and again to merge into span attributes. It now reads the context attributes once into a local variable and passes it to both uses.Dynamic attributes preserved
db.instance.id(@connected_host) is added per-call since it can change after connection (e.g., after failover)db.statementis inherently per-queryTrilogy.attributes(context propagation) is per-callBenchmarks
client_attributes micro-benchmark
Measured with
Process.clock_gettimeover 500k iterations. Test setup:Trilogy.allocatewith typical connection options (host, database, username),db_statement: :omitconfig.client_attributes(no sql)client_attributes(with sql, omit)database_namedatabase_userHash construction comparison
Comparing strategies for building the per-query attributes hash:
Hash#dupfrom frozen cached hash — new approachThe cached
dupapproach is ~45% faster for hash construction alone, but the real win comes from eliminating the repeated method calls (database_name,database_user,config[:peer_service],connection_options.fetch) that populated the hash on every call.Test coverage
patches/client_attributes_test.rb) with 19 tests covering:database_nameanddatabase_useraccessor behavior