open-telemetry / open-telemetry/opentelemetry-ruby-contrib
Migrate Rails instrumentation to use only ActiveSupport::Notifications
@ahayworth is already working on this.
Since Dec 6, 2022.
- Dominant language
- Ruby
- Stars
- 137
- Forks
- 258
- Avg merge
- 3d 17h
- Merged PRs (30d)
- 30
Description
We recently uncovered an issue wherein our action_pack instrumentation caused a production problem. There are many ways to consider such an issue, but one way is to acknowledge that monkey-patching is fraught with peril, and that we should consider different methods of instrumentation.
For Rails, we should be able to use ActiveSupport::Notifications. We already use this to great effect in our action_view instrumentation, and we think we can expand this further to the rest of our Rails instrumentation.
To do this, we need to:
- Verify that Rails 7 implements safer subscription start/end behavior (a crashing subscriber should not prevent us from finishing our spans correctly).
- Audit the instrumentation we have for Rails, and figure out what ActiveSupport notifications could provide similar levels of instrumentation:
- action_pack
process_action.action_controlleris probably the right one.payload.responseandpayload.statuswill be set when the notification finishes- This is defined in actionpack/lib/action_controller/metal/instrumentation.rb
- action_view
- This one is already done 🎉
- active_job
- We can't drop our patch for serializing metadata regardless of what we choose to do here.
- We implement our enqueue instrumentation as an
around_enqueuefilter; which is precisely what Rails does. perform.active_jobwill likely cover the rest.
- active_record
- This was one of the things preventing us from doing notifications the first time around: we only get
sql.active_recordandinstantiation.active_record. sql.active_recordis great, but it's low-level and won't capture whether we are doing a#findor a#destroy, etc. Nor will it capture callbacks.instantiation.active_recordwill capture callbacks, but it's pretty basic and still doesn't tell us what method we're really in.
- This was one of the things preventing us from doing notifications the first time around: we only get
- active_support
- This is already done, because it doesn't do anything on its own! 🎉
- It's a base for using notifications for spans, which is precisely what we want.
- Basically, previous contributors thought ahead to a future where it would be useful to have a generic "subscribe, make a span" kind of thing. Yay!
- action_pack
- Determine how many (if any) notifications are generated entirely after the instrumented operation is completed, and how many are generated around the instrumented operation.
- This has implications around accessing the
current_span, which may be a deal-breaker.
- This has implications around accessing the
- Augment our custom span subscriber in any way necessary to support a more broad use-case than what it does now
- We need to figure out if there is a better way to do this in Rails 7 and if we should adjust this span subscriber.
- We also should look and see if we could backport the Rails 7 fixes here ... or get that backported upstream.
- Work through instrumentation and convert them.
- Depending on the outcome of the safety/backport investigations, we can decide whether or not we wish to keep the monkey-patched versions around for older Rails installations or not.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.