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

Add dalli support #196

Merged
merged 2 commits into from
Sep 30, 2017
Merged

Add dalli support #196

merged 2 commits into from
Sep 30, 2017

Conversation

p-lambert
Copy link
Member

@p-lambert p-lambert commented Sep 22, 2017

Overview

Dalli integration will trace all calls to your memcached server:

require 'dalli'
require 'ddtrace'

# patch Dalli so all clients are traced
Datadog::Monkey.patch_module(:dalli)

client = Dalli::Client.new('localhost:11211', options)
client.set('abc', 123)

# to change Dalli service name, use the Pin class
pin = Datadog::Pin.get_from(client)
pin.service = 'primary-cache'

@p-lambert p-lambert added the do-not-merge/WIP Not ready for merge label Sep 22, 2017
@palazzem palazzem added this to the 0.9.0 milestone Sep 23, 2017
@palazzem palazzem added the integrations Involves tracing integrations label Sep 23, 2017
@p-lambert p-lambert force-pushed the pedro/add-dalli-support branch 5 times, most recently from 6e1d919 to 231b553 Compare September 26, 2017 19:27
@palazzem palazzem self-requested a review September 26, 2017 23:03
@palazzem palazzem removed the do-not-merge/WIP Not ready for merge label Sep 26, 2017

module Datadog
module Contrib
module Dalli
Copy link
Contributor

Choose a reason for hiding this comment

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

@p-lambert let's write a bit of documentation for this contrib module.

@p-lambert p-lambert force-pushed the pedro/add-dalli-support branch 2 times, most recently from 39a3c91 to 5b82c79 Compare September 28, 2017 18:34
Dalli integration will trace all calls to your `memcached` server:

require 'dalli'
require 'ddtrace'
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need to patch / patch all something?

Copy link
Contributor

Choose a reason for hiding this comment

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

will add that as a general example; unfortunately for Rails it will be:

require 'ddtrace'

# Tracing configuration
Rails.configuration.datadog_trace = {
  auto_instrument: true,
  auto_instrument_redis: true,
  auto_instrument_grape: true,
  default_service: ENV['RAILS_SERVICE'] || 'rails-local',
  default_database_service: ENV['RAILS_MYSQL_SERVICE'] || 'db-local',
  default_cache_service: ENV['RAILS_CACHE_SERVICE'] || 'cache-local',
  default_controller_service: ENV['RAILS_CONTROLLER_SERVICE'] || 'controller-local',
  trace_agent_hostname: ENV['DATADOG_TRACER'] || 'datadog'
}

Datadog::Monkey.patch_module(:dalli)
pin = Datadog::Pin.get_from(::Dalli)
pin.service = 'dalli-cache'

mostly because we have some things that are patched via Rails settings, and others that are not. I'd say to not add Dalli to the auto_instrument_* since we're planning to remove this settings in favor of a better way to configure our integrations.

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.

Let's wait for the CI that is green and merge it. I've updated the docs so the patch_module call is here, and we show also how to update the service name.

After the Configuration change, I think it will be far better. For now, let's stick to the legacy API.

@palazzem palazzem merged commit 3d45158 into master Sep 30, 2017
@palazzem palazzem deleted the pedro/add-dalli-support branch September 30, 2017 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants