feat: Trilogy: introduce record_exception setting #1653
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
ericmustin
left a comment
ericmustin
left a comment
There was a problem hiding this comment.
Looks fine to me, happy to approve , We have some prior patterns like this in other instrumentation(ie rack/rails), and this seems like a useful knob to make the instrumentation more flexible. I am not an active trilogy user at this time so I defer to the folks working with that gem daily.
| @connection_options = options # This is normally done by Trilogy#initialize | ||
|
|
||
| tracer.in_span( | ||
| in_span( |
There was a problem hiding this comment.
@fbogsany suggested we improve the in_span convenience method to allow for the desired behaviour so all that is required in the PR is to update the calls to in span.
There was a problem hiding this comment.
@kirs - what do you think about updating this PR to use the changes to the in_span method from open-telemetry/opentelemetry-ruby#1911?
|
@kirs sorry for the slow turnaround but if you update the gemspec dependency of the |
kirs
force-pushed
the
trilogy-record_exception-setting
branch
from
05258be to
be696db
Compare
instrumentation/trilogy/opentelemetry-instrumentation-trilogy.gemspec
Outdated
Show resolved
Hide resolved
fbogsany
changed the title
kirs
force-pushed
the
trilogy-record_exception-setting
branch
from
0f59835 to
604e35d
Compare
5 participants
In apps that use advanced exception tracking, having raw Trilogy error on the span can be misleading because it's likely handled by ActiveRecord. Surfacing it as an actual exception introduces some misdirection here.
I'd like to suggest that we make this behaviour configurable via
record_exceptionoption. It would be set totrueby default, preserving existing behaviour.@robertlaurin @fbogsany