open-telemetry / open-telemetry/opentelemetry-ruby-contrib

Migrate Rails instrumentation to use only ActiveSupport::Notifications

Open
#218 26 comments 5 reactions 1 assignee View on GitHub

@ahayworth is already working on this.

Since Dec 6, 2022.

feature help wanted instrumentation instrumentation-action_pack instrumentation-action_view instrumentation-active_job instrumentation-active_record instrumentation-active_support keep
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
    • action_view
    • 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_enqueue filter; which is precisely what Rails does.
      • perform.active_job will 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_record and instantiation.active_record.
      • sql.active_record is great, but it's low-level and won't capture whether we are doing a #find or a #destroy, etc. Nor will it capture callbacks.
      • instantiation.active_record will capture callbacks, but it's pretty basic and still doesn't tell us what method we're really in.
    • 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!
  • 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.
  • 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.