dd-trace-rb

dd-trace-rb copied to clipboard

Current behaviour

Rails callbacks (before/after) are not a part of the action span. In the before_action callback, active_span is:

SpanOperation(name:rack.request,sid:1008860920845926479,tid:1866324838581500214,pid:0)

But in the action call:

SpanOperation(name:rails.action_controller,sid:3701457545195306293,tid:1866324838581500214,pid:1008860920845926479)

Expected behaviour

active_pack spans should include the callbacks. That could be done either through a separate span or by extending the current one.

I'm raising this as a bug rather than a feature request, because otherwise we end up in situations where (for example) rack seems to own spans from active_record, which is not true: Those queries are made in the before_action on the controller.

This is also very important when trying to name the spans something more specific - for example when a top level ApiController that other things inherit knows how to label the span in before_action, but delegating that to all specific Api implementations would be extremely repetitive.

Steps to reproduce

class SomeController < ApplicationController
  before_action :bar

  def foo
    Datadog::Tracing.trace("this.goes.in.controller") {}
  end

  private
  
  def bar
    Datadog::Tracing.trace("this.goes.in.rack") {}
  end
end

Environment

• ddtrace version: 1.1.0
• Configuration block (Datadog.configure ...):

    c.tracing.instrument :rails, service_name: 'marketplace'
    c.tracing.instrument :rack, request_queuing: true, web_service_name: 'marketplace-rack'

• Ruby version: 2.7
• Operating system: linux
• Relevant library versions:

ActionPack instrumentation doesn't cover before/after_action; this is known behavior. We wanted to add coverage for this, but the nature of how it is implemented in Rails (as an ActiveSupport notification) makes it incredibly difficult. See this ancient PR: https://github.com/DataDog/dd-trace-rb/pull/333

Is there a recommended way to deal with relabeling controllers that doesn't require explicit copying the code inside all of the leaf controllers? We're migrating from newrelic and used NewRelic::Agent.set_transaction_name in before_action. This doesn't work here, since that renames the Rack span instead. I could imagine some hack that puts it in after_action and browses previous spans to change the right one, but... that's a mess.

Can you clarify on the desired behavior? You want to change the resource on the rails.action_controller span from within a before/after_action?

Generally speaking most users have preferred the controller resource we provide: for what purpose do you want to rename it?

There may be some options like set_transaction_name we may be able to provide, but I just want to know a little more about the intent first.

So I've got an ApiController base class which knows how to name the resource correctly, but doesn't actually implement any endpoint. Only controllers inheriting from it actually have the actions and I don't want to replicate the resource naming in them. The problem is that the Class#method naming in the API classes doesn't really map nicely to the requests (either being too general or too specific), so I want to use a custom name that depends on request parameters.

I can't (as far as I know) use the span processor / filter here, because I don't have access to the parameters anymore.

After some playing around last week, I ended up with renaming the trace instead. Specifically this works: (after so that action_pack doesn't override it)

after_action :set_tracing_transaction_name
def set_tracing_transaction_name
   ...
   Datadog::Tracing.active_trace&.resource = "ApiChunk:#{transaction_name}"
end

So at this point I'd like to rename the controller span, but it became more of a "nice to have". I can live with only the trace being renamed.

I was going to recommend exactly what you did (renaming the trace resource). I think that should serve as an okay interim option.

Generally speaking, I'd like to make the instrumentation for packages like Rails a little more customizable through some kind of middleware, DSL or other mechanism. However, I want to plan carefully how we introduce this as a public API, so it may be a while till we do something like this.

Another fun side effect of this is that if a before_action renders or raises, it never hits the actionpack instrumentation (because it's patched into ActionController::Metal, and it's therefore lower in the ancestor chain of the controller than AbstractController::Callbacks which gets mixed into ActionController::Base (which is a subclass of Metal).

i.e. the ancestor chain looks like...

MyFunController
ActionController::Base # We inherit from this
AbstractController::Callbacks # This is included into ActionController::Base
Datadog::Tracing::Contrib::ActionPack::ActionController::Metal # This is prepended onto ActionController::Metal
ActionController::Meta

When process_action gets called, the before callbacks get run in AbstractController::Callbacks before it calls super, and so if it renders or raises, Datadog::Tracing::Contrib::ActionPack::ActionController::Metal#process_action is never called.

I mostly care about this because the actionpack instrumentation is responsible for setting the resource name of the top-level Rack span to be the controller name. This means we wind up with a mountain of traces simply called Zendesk::Application#GET, which is not terribly helpful (that happens because the Rack instrumentation patches the middleware name into all of our middlewares and our Rails application is the final "Middleware" from Rack's point of view.

@KJTsanaktsidis Right, because its the ActionController instrumentation that's updating the trace resource with a controller name hasn't run yet before you're raising an error.

In theory, this could be done at a routing level if we instrumented the routing behavior. That would always run before any controllers, but should be able to show a route name.

Yeah instrumenting the router for this would make sense actually

@KJTsanaktsidis Cool. It's a bit separate from "instrument before/after actions", so we could open that as its own feature request.

Hello everyone 👋

I'm bringing this up again because the two problems described here are two major issues for us:

1. before/after action not included in the rails.action_controller span
2. missing resource name on action that stop in middleware or before_action

For 2, I have open a dedicated issue for it. I'm sorry @delner I opened it as a "bug report", and just saw you proposed to open it as a "feature request".

For 1, I'll be looking this week to see if it's possible to push upstream instrumentation around before/around/after action. That should make thing easier?