Migrate `rails` configuration to new API #224

p-lambert · 2017-10-12T22:05:09Z

This PR migrates the rails integration to the new configuration API, so we don't rely on the Rails.configuration object anymore.

The motivation for this is to provide a common, standardized way of configuring all integrations.

Currently rails integration is configured through:

Rails.configuration.datadog_trace = {
  auto_instrument: true,
  auto_instrument_redis: true,
  default_service: 'my-rails-app'
}

From now on the configuration should be done through:

Datadog.configure do |c|
  c.use :rails, auto_instrument: true, auto_instrument_redis: true, default_service: 'my-rails-app`
end

Notice that we're currently supporting both configuration modes for now (this change is backwards-compatible) but support for for the Rails.configuration object should be dropped soon.

palazzem

minor nitpicks and questions otherwise it's good in the overall. The only stopper here is that I remember we decided to switch from c.use to c.instrument. I'm fine with both, though I prefer a c.instrument :rails.

If you agree, we can just work on top of c.use, and then write a last PR that updates this everywhere (just to avoid writing a PR and rebasing all of them on top of the new one).

palazzem · 2017-10-30T10:55:33Z

lib/ddtrace.rb

@@ -46,41 +46,3 @@ def configure
 end

 require 'ddtrace/monkey'
-
-# Datadog auto instrumentation for frameworks


palazzem · 2017-10-30T10:59:08Z

lib/ddtrace/contrib/rack/middlewares.rb

+
+        option :tracer, default: Datadog.tracer
+        option :default_service, default: 'rack'
+        option :distributed_tracing_enabled, default: false

        def initialize(app, options = {})
          # update options with our configuration, unless it's already available
          [:tracer, :default_service, :distributed_tracing_enabled].each do |k|


should we remove this? I think we can simply merge them no?

I agree, but I wonder if we shouldn't do these improvements on another PR. My idea was to migrate everything to the new API and then do a thorough cleanup in the configuration code. Let me know if you prefer to do this in this PR.

palazzem · 2017-10-30T11:02:33Z

lib/ddtrace/contrib/rails/patcher.rb

+        option :trace_agent_port, default: Datadog::Writer::PORT
+        option :env, default: nil
+        option :tags, default: {}
+        option :sidekiq_service, default: 'sidekiq'


shouldn't be in the sidekiq patcher?

absolutely. this is only here for now to maintain backward compatibility!

palazzem · 2017-10-30T11:40:41Z

lib/ddtrace/contrib/rails/patcher.rb

+        include Base
+        register_as :rails, auto_patch: true
+
+        option :enabled, default: true


We've a different logic here: https://github.com/DataDog/dd-trace-rb/pull/226/files#diff-acecea54cdb8458166cefacb04d7a9baR26

Should we do the same? Because in Rails we're disabling / enabling the tracer when the Railtie is executed, but not when the initializer is called.

It means that you set the Tracer.enabled <- False but the tracer can still send traces until Rails isn't fully initialized. Am I missing something?

For the future, it would be great accessing directly to the tracer so that tracer specific configurations aren't part of integrations. That way as we've discussed before, you configure the Tracer AND Rails.

I agree with everything you pointed out, but I'd keep things as they're now to avoid changing too many things at once. Right now we're maintaining exactly the same behavior as before. The next step would be getting rid of the Framework.configure method entirely, by using the pattern you described (using the custom setters provided in the new API)

p-lambert · 2017-11-01T19:09:18Z

@palazzem about the use/instrument thing, I also prefer instrument. Considering we'll be able to configure also the tracer, I'd create an alias and keep both so we could have:

Datadog.configure do |c|
  c.use :tracer, logger: Logger.new(STDOUT)
  c.instrument :rails
end

What do you think? Actually it could be anything else. I just think that one name work well for the integrations AND the tracer. Let me know if you have any thoughts on that.

palazzem · 2017-11-15T17:50:52Z

lib/ddtrace/contrib/rack/middlewares.rb

+
+        option :tracer, default: Datadog.tracer
+        option :default_service, default: 'rack'
+        option :distributed_tracing_enabled, default: false

        def initialize(app, options = {})
          # update options with our configuration, unless it's already available
          [:tracer, :default_service, :distributed_tracing_enabled].each do |k|


palazzem · 2017-11-15T17:53:18Z

lib/ddtrace/contrib/rails/patcher.rb

+        option :trace_agent_port, default: Datadog::Writer::PORT
+        option :env, default: nil
+        option :tags, default: {}
+        option :sidekiq_service, default: 'sidekiq'


palazzem · 2017-11-15T18:01:22Z

lib/ddtrace/contrib/rails/patcher.rb

+        include Base
+        register_as :rails, auto_patch: true
+
+        option :enabled, default: true


palazzem

All good to me after the rebase!

p-lambert added the do-not-merge/WIP Not ready for merge label Oct 12, 2017

p-lambert force-pushed the pedro/create-configurable-module branch from 8da0e36 to 35b13b2 Compare October 12, 2017 22:22

p-lambert force-pushed the pedro/migrate-configurations branch 11 times, most recently from 082dd2d to ba7a27a Compare October 16, 2017 17:43

p-lambert force-pushed the pedro/create-configurable-module branch 2 times, most recently from a97420c to b4a67e4 Compare October 16, 2017 19:18

p-lambert force-pushed the pedro/migrate-configurations branch from ba7a27a to 6e629a6 Compare October 16, 2017 19:34

p-lambert force-pushed the pedro/create-configurable-module branch from b4a67e4 to 66bb372 Compare October 16, 2017 20:08

p-lambert force-pushed the pedro/migrate-configurations branch from 6e629a6 to 9454601 Compare October 16, 2017 20:45

p-lambert force-pushed the pedro/create-configurable-module branch from 66bb372 to 19c2020 Compare October 16, 2017 21:22

p-lambert force-pushed the pedro/migrate-configurations branch from 9454601 to 7182451 Compare October 16, 2017 21:24

p-lambert force-pushed the pedro/create-configurable-module branch 2 times, most recently from 3485012 to bf5a998 Compare October 16, 2017 22:02

p-lambert force-pushed the pedro/migrate-configurations branch 2 times, most recently from 3783f45 to 996453e Compare October 16, 2017 22:56

p-lambert changed the title ~~Migrate configurations to new API~~ Migrate rails configuration to new API Oct 17, 2017

p-lambert removed the do-not-merge/WIP Not ready for merge label Oct 17, 2017

palazzem added this to the 0.10.0 milestone Oct 19, 2017

palazzem changed the base branch from pedro/create-configurable-module to master October 19, 2017 16:21

palazzem self-requested a review October 19, 2017 16:21

palazzem added the core Involves Datadog core libraries label Oct 19, 2017

palazzem added the integrations Involves tracing integrations label Oct 19, 2017

palazzem reviewed Oct 30, 2017

View reviewed changes

p-lambert force-pushed the pedro/migrate-configurations branch 2 times, most recently from 40529c5 to b64075e Compare November 1, 2017 21:19

palazzem previously approved these changes Nov 15, 2017

View reviewed changes

palazzem dismissed their stale review via bd457a9 November 15, 2017 17:59

palazzem force-pushed the pedro/migrate-configurations branch from b64075e to bd457a9 Compare November 15, 2017 17:59

palazzem previously approved these changes Nov 15, 2017

View reviewed changes

lib/ddtrace/contrib/rails/patcher.rb

include Base

register_as :rails, auto_patch: true

option :enabled, default: true

Copy link

Contributor

palazzem Nov 15, 2017

Choose a reason for hiding this comment

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

👍

Migrate rails configuration to new API

4c88a0d

p-lambert dismissed palazzem’s stale review via 65693b8 November 21, 2017 22:04

p-lambert force-pushed the pedro/migrate-configurations branch from bd457a9 to 65693b8 Compare November 21, 2017 22:04

Turn rails into regular contribution

cac9773

p-lambert force-pushed the pedro/migrate-configurations branch from 65693b8 to cac9773 Compare November 21, 2017 22:45

palazzem approved these changes Nov 23, 2017

View reviewed changes

palazzem merged commit 0f1780b into master Nov 23, 2017

palazzem deleted the pedro/migrate-configurations branch November 23, 2017 13:16

palazzem mentioned this pull request Jan 11, 2018

Refactor request: objections to moving all reads of Rails.configuration.datadog_trace to the Railtie #188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `rails` configuration to new API #224

Migrate `rails` configuration to new API #224

p-lambert commented Oct 12, 2017 •

edited

Loading

palazzem left a comment

palazzem Oct 30, 2017

palazzem Oct 30, 2017

p-lambert Oct 30, 2017

palazzem Nov 15, 2017

palazzem Oct 30, 2017

p-lambert Oct 30, 2017

palazzem Nov 15, 2017

palazzem Oct 30, 2017

palazzem Oct 30, 2017

p-lambert Nov 1, 2017

palazzem Nov 15, 2017

p-lambert commented Nov 1, 2017

palazzem Nov 15, 2017

palazzem Nov 15, 2017

palazzem Nov 15, 2017

palazzem left a comment

Migrate rails configuration to new API #224

Migrate rails configuration to new API #224

Conversation

p-lambert commented Oct 12, 2017 • edited Loading

palazzem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-lambert commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palazzem left a comment

Choose a reason for hiding this comment

Migrate `rails` configuration to new API #224

Migrate `rails` configuration to new API #224

p-lambert commented Oct 12, 2017 •

edited

Loading