patch process_action including filters

[HoneyryderChuck]

This is in reference to this action here: https://github.com/DataDog/dd-trace-rb/blob/b69dca3532453e99b5bd26183bf89bedeab9a8aa/lib/ddtrace/contrib/rails/action_controller_patch.rb .

I'm encountering this inconsistency, at least according to my expectation, where the action controller tracer only pertains to the action itself. I think it makes sense that this tracer should include the filters (before_action, after_action).

My use case is: I want to augment the current span in an after_action, instead of "polluting" the controller action with datadog tracer calls. However I can't do this, because there is no active_span when the after action is triggered.

[delner]

Yup, we're aware of this not playing well with before_action/after_action. This is because of the class we currently patch (ActionController::Base) doesn't allow for this.

We will likely add additional instrumentation into the ActionController::Metal class such that we'll see a parent span for the controller's dispatch, then a child span for its actual action. This will form a better foundation for modeling traces of a controller's life-cycle.

After that, we hopefully can add more spans around these before_action/after_action hooks, which would be children of the dispatch span, but that part is TBD. If I remember correctly, the ActionController uses ActiveSupport callbacks to power these hooks, and its a bit of a mess to patch: because you're not wrapping nice methods on classes, but abstract blocks in the ActiveSupport internals which are very unstable. Such kind of patching expensive to maintain and dangerous to user's applications, which is why it doesn't exist right now. We'll re-evaluate this again soon, see if there's a better way.

For now, we're working on refactoring our Rails integration a bit which is a pre-requisite for this, so watch for those updates first. This issue would follow those changes.

[delner]

We've overhauled our Rails instrumentation a bit in 0.27.0, so we should begin work on changing how we instrument controller actions soon; look for that in the near future.

[krasnoukhov]

I just stumbled upon this limitation, is there any update whether it's going to be prioritized?

[krasnoukhov]

Looks like #669 will address this but it seems pretty outdated

[ErvalhouS]

Any development coming this way? I have this very same issue in my project.

[delner]

I would like to work on this but its not currently on the schedule; I'll follow up with the team and see what they'd like to do about it.

[stephenhyde]

Just want to confirm that this is the issue I am seeing as well. On most of my routes i have a before action to verify their session. This common code lives in the api_controller. If the session fails to verify, I render a 401 and short circuit. I found the spans in APM under GET 401 and not on their respective routes. Is this related?

[delner]

@stephenhyde Yeah, it sounds like that short-circuiting would be related to this.

I've rebumped this to "Needs Triage"; we'll discuss this issue in our next planning meeting.

[JarredLHumphrey]

We're having this issue as well. Any update on this? Would love to have tracing for our before/after hooks.

[delner]

Last time I took a crack at this (years ago), I found this wasn't practically possible. It's an issue with how Rails designs its code.

Rather than implement a first-class before_action, after_action method, they dynamically define these methods and pipe them through the ActiveSupport callback framework. To hack this would mean writing a bunch of monkey-patches on a bunch of private methods/classes deep in that library...

This is probably not a good idea: if Rails changes their internal implementation in a patch fix or minor release (which they can do without bumping to a major version by rule of semantic versioning), our implementation will break, and possibly so will users' apps. Stability and unobtrusiveness is a principle we must uphold... we cannot take that kind of risk.

If there's a less risky/invasive means of accomplishing this, then I'm all for it. Community contributions are welcome here.

[zserafini]

Not an overly helpful comment 😅 , but I wanted to voice additional support for a fix here (I know, it's not an easy one 😞 )

I think it's a fairly common pattern to use ActionController callbacks in Rails to support cross controller concerns like auth checks. Since grouping APM Service metrics (at least I think?) seem to default to rack.request, the traces on these usually errant requests are not grouped with their respective routes - which leads to hiding real issues when trying to monitor/debug specific routes.