Report isolation level on ActiveRecord transaction #1672
Conversation
kirs
requested review from
a team,
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd,
robertlaurin,
simi and
xuan-cao-swi
as code owners
| tracer.in_span('ActiveRecord.transaction', attributes: { 'code.namespace' => name }) do | ||
| super | ||
| def transaction(*args, **kwargs, &block) | ||
| attributes = { 'code.namespace' => name } |
There was a problem hiding this comment.
nit:
attributes = {
'code.namespace' => name,
'db.transaction_isolation' => kwargs[:isolation]
}.compact!
There was a problem hiding this comment.
@xuan-cao-swi this would potentially set the attributes hash to nil.
https://ruby-doc.org/3.4.1/Hash.html#method-i-compact-21
compact! → self or nil
Returns self with all its nil-valued entries removed (in place):
Returns nil if no entries were removed.
| attributes['db.transaction_isolation'] = kwargs[:isolation].to_s | ||
| end | ||
| tracer.in_span('ActiveRecord.transaction', attributes: attributes) do | ||
| super(*args, **kwargs, &block) |
arielvalentin
left a comment
arielvalentin
left a comment
There was a problem hiding this comment.
Thanks for this submission. Before we can accept it though, the tests must pass.
The tests are currently failing https://github.com/open-telemetry/opentelemetry-ruby-contrib/actions/runs/17834374868/job/51063006548#step:3:191
| def transaction(*args, **kwargs, &block) | ||
| attributes = { 'code.namespace' => name } | ||
| if kwargs[:isolation] | ||
| attributes['db.transaction_isolation'] = kwargs[:isolation].to_s |
There was a problem hiding this comment.
Nit: There is no semantic convention for this.
In practice only 1 other instrumentation implements it and namespaces it under the specific DB vendor since I believe Isolation Levels are rather specific to ACID SQL style databases.
Having said all that, I think that its fine to be under the db namespace but name it db.tansaction_isolation_level for symmetry with the existing usage.
🙏🏼 if you would kindly submit a PR to add this semconv attribute upstream.
Do you all use db client spans as well? Would you like this attribute to appear on those spans as well or is this span good enough?
There was a problem hiding this comment.
I would go with the attribute naming db.tansaction.isolation_level so that we could also have db.tansaction.id. I also suggest raising a pr to add it to semconv.
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
5 participants
This is a very useful field if your app is in progress of switching in between isolation levels.