Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveSupport::Notifications subscriptions #380

Merged
merged 6 commits into from
Mar 26, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Mar 21, 2018

There are numerous integrations that depend on ActiveSupport::Notifications to perform tracing. e.g. ActiveRecord, Racecar, Sequel, etc. Currently each of these integrations have their own implementations.

In the interest of creating some consistency, and encouraging new integrations that require ActiveSupport::Notifications, this pull request adds a Datadog::Contrib::ActiveSupport::Notifications::Subscriber module, that can be composed into Patchers or other components that want to instrument ASN events with Datadog tracing.

(This pull request is an updated version of #324; thanks @dasch for his contribution!)

A hypothetical ActiveRecord example (specific patterns, payloads may not be totally accurate):

require 'active_support/notifications'
require 'datadog/contrib/base'

module Patcher
  include Base
  include ActiveSupport::Notifications::Subscriber

  # Setup your subscriptions in this block...
  # Will be called at most, once. Prevented from running multiple times,
  # which avoids duplicate subscriptions for the same events.
  on_subscribe do
    # Pattern, span name, trace options (default {}), tracer (default Datadog.tracer), callback
    subscribe('sql.active_record', 'mariadb.query', {}, configuration[:tracer], &method(:sql))
    subscribe('instantiation.active_record', 'activerecord.instantiation', {}, configuration[:tracer], &method(:instantiation))
  end

  module_function

  def patch
    # Invokes #on_subscribe.
    subscribe!
  end

  def sql(span, name, id, payload)
    # Be sure to give it a resource name!
    span.resource = payload[:sql]
    span.set_tag('cached', payload[:cached])
  end

  def instantiation(span, name, id, payload)
    # Be sure to give it a resource name!
    span.resource = payload[:class_name]
    span.set_tag('record_count', payload[:record_count])
  end
end

Also allows for lazy subscriptions (useful if you want to trace two different events the same way):

on_subscribe do
  # The #subscription method builds a subscription object
  # Span name, trace options (default {}), tracer (default Datadog.tracer), callback
  subscription('racecar.process', {}, configuration[:tracer], &method(:process)).tap do |subscription|
    # Actually subscribe it to ActiveSupport::Notifications.
    subscription.subscribe('process_message.racecar')
    subscription.subscribe('process_batch.racecar')
  end
end

Also see #381 for an example of an actual integration using this.

@delner delner added integrations Involves tracing integrations community Was opened by a community member feature Involves a product feature labels Mar 21, 2018
@delner delner self-assigned this Mar 21, 2018
@delner delner changed the base branch from master to 0.12-dev March 21, 2018 20:05
@delner delner force-pushed the feature/active_support_notifications_subscriber branch from ce012bf to cce5bed Compare March 21, 2018 20:34
@delner
Copy link
Contributor Author

delner commented Mar 21, 2018

@dasch would you like to review this?

Copy link
Contributor

@dasch dasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@delner delner force-pushed the feature/active_support_notifications_subscriber branch from cce5bed to cce6a81 Compare March 22, 2018 16:02
@palazzem palazzem added this to the 0.12.0 milestone Mar 26, 2018
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we add a new functionality without altering the rest of the tracer, we can safely add it in the next minor release.

@palazzem palazzem merged commit 9afa4a3 into 0.12-dev Mar 26, 2018
@palazzem palazzem deleted the feature/active_support_notifications_subscriber branch March 26, 2018 08:58
palazzem pushed a commit that referenced this pull request Mar 26, 2018
@rromanchuk
Copy link

@delner I feel like this is a stupid question, but how would one go about testing this really quick without getting to deep into

ActiveSupport::Notifications.subscribe(FastJsonapi::ObjectSerializer::SERIALIZABLE_HASH_NOTIFICATION) do |name, start, finish, id, payload|
  # send to DD
end

Would I just do something like this in the block?

span = Datadog.tracer.trace('my-trace')
  span.resource = payload[:name]
  span.start_time = start
  span.finish(finish)

@delner
Copy link
Contributor Author

delner commented Nov 8, 2018

@rromanchuk You could do something like that in the block, you would basically be making a span retroactively after a finished event. Just be sure to set a service, too.

This subscription pattern has matured a bit since we first introduced it. We made an Datadog::Contrib::ActiveSupport::Notifications::Event module, based on this PR, that you could use to more easily instrument ActiveSupport events (granted it's probably not as simple as what you have there.) A number of our integrations have since implemented this Event pattern, including Racecar, ActiveModelSerializers, ActiveRecord, so you could look at those as a reference.

A simple implementation of Event this might look something like:

class SerializableHashEvent
  include ActiveSupport::Notifications::Event

  def event_name
    FastJsonapi::ObjectSerializer::SERIALIZABLE_HASH_NOTIFICATION
  end

  def span_name
    'my-trace'
  end

  def process(span, event, id, payload)
    # You can add tags or modify the span here.
  end
end

SerializableHashEvent.subscribe!

@rromanchuk
Copy link

@delner cool thanks for the guidance. I'll eventually take a look how these services were implemented and clean this up instead of monkey patching with an initializer. Ideally, submit a datadog integration directly to fast_jsonapi https://github.com/Netflix/fast_jsonapi#instrumentation

Just be sure to set a service

If i'm also using the the rails integration c.use :rails will it pick up that default on its own or do i still need to set it since? i'm guessing it's just an attribute on the span span.service.. i need to rtfm.

On that note, although i'm still in the process of learning as i'm setting up my integration, i find myself struggling with best practices on naming conventions, but I think it will become more clear once I start interacting with APM, in this case i'm assuming i want my service name to be identical to my rails app as this is really just the last mile of my request? This is where i'm at

ActiveSupport::Notifications.subscribe(FastJsonapi::ObjectSerializer::SERIALIZABLE_HASH_NOTIFICATION) do |name, start, finish, id, payload|
    # name is render.fast_jsonapi.serializable_hash
    # payload is {:name=>"MySerializer"}
    span = Datadog.tracer.trace(name)
    span.service = "MyRailsApp"
    span.resource = payload[:name]
    span.span_type = "http"
    span.start_time = start
    span.finish(finish)
  end

@delner
Copy link
Contributor Author

delner commented Nov 9, 2018

You still need to set service via span.service; it's not something that's auto-inherited, at least not in a way that you'd probably want to see.

Looking at other integrations will likely be the best way of understanding the conventions, but as a really short cheatsheet:

  • name: Name of the operation/function being done. This should never vary based off variable data and is usually hardcoded. Should be all lowercase with period delimiters e.g. mysql.query or json.encode.
  • resource: Application domain name for what's being operated on. Acts as a group name. Usually a route, job name, or other domain specific value that groups well. Can appear in any format e.g. UpdateBillingJob, /article/show.
  • service: Logical group name that generates spans, usually an application name or library name. Traces with same service name will be aggregated and treated as if they are one application in the UI. Should be lowercase and spaced with dashes, e.g. images-api, billing-worker, article-db, graphql, etc. Integrations should always provide some kind of service name by default, and this value should be configurable by the user.
  • span_type: Well-known type of operation, e.g. http, sql. Used by the UI to create nicer presentation of certain spans. Optional; do not give arbitrary values.

Given those, I'd recommend changing your service name a bit, maybe give it a service name like my-rails-app-fastjsonapi, otherwise the basics look okay.

@rromanchuk
Copy link

Thanks that helps. It immediately became clear to me i wanted a unique service name the second i looked at the flame graph. Looks like i need to clean up the rest of my service names too, makes more sense using it in practice. I was conflating the entire HTTP API request as a single service, which I guess technically could be desired depending on the scope.

screen shot 2018-11-09 at 5 56 02 pm

Looks like I accidentally introduced duplicate spans. I've got redis and a cache service that are tracing the same thing. I probably was confused during initial configuration and didn't realize the rails integration already included that service.

One last question i promise. I realized i had APM configuration settings in my ansible deploy for my datadog role, but i completely forgot why and how I added them. Looks like some type of filter, can i completely remove this? Maybe it's just legacy, because i don't see any usage or examples in https://github.com/DataDog/datadog-agent/blob/master/pkg/config/config_template.yaml

apm_config:
    enabled: true
    analyzed_spans:
      grape|grape.endpoint_run: 1
      sparcore|rack.request: 1
      net/http|http.request: 1
      aws|aws.command: 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants