[rails] add the TraceMiddleware immediately instead of relying in another initializer #208
Conversation
palazzem
added
enhancement
integrations
| @@ -72,6 +72,7 @@ of the Datadog tracer, you can override the following defaults: | |||
| default_controller_service: 'rails-controller', | |||
| default_cache_service: 'rails-cache', | |||
| default_database_service: 'postgresql', | |||
| distributed_tracing_enabled: false, | |||
There was a problem hiding this comment.
we missed that in the doc; mostly the goal is to remember that with the current config version, you can enable distributed tracing from the settings. Not that great since we're planning to change that, but at least we have a reference for that.
| @@ -36,11 +36,13 @@ def self.tracer | |||
| module Datadog | |||
| # Railtie class initializes | |||
| class Railtie < Rails::Railtie | |||
| # add instrumentation middlewares | |||
| options = {} | |||
| config.app_middleware.insert_before(0, Datadog::Contrib::Rack::TraceMiddleware, options) | |||
There was a problem hiding this comment.
this change ensures that the middleware is here; with the previous approach, applications may ignore the initializer resulting in wrong traces
| @@ -38,9 +38,9 @@ def initialize(app, options = {}) | |||
| @options = options | |||
| end | |||
|
|
|||
| def configure | |||
| def configure(reload = false) | |||
There was a problem hiding this comment.
mostly a problem with the current concept of configuration; with the new Configuration it should be easy to update the Rack middleware configurations without reinitializing from scratch Rails from our tests. Consider that this reload parameter is here only for testing purposes (and it's not ideal).
| @@ -58,7 +58,8 @@ def configure | |||
| # rubocop:disable Metrics/MethodLength | |||
| def call(env) | |||
| # configure the Rack middleware once | |||
| configure() | |||
| configure | |||
| clean_context | |||
There was a problem hiding this comment.
we can clean the context all the time; another PR makes this process safe even if the @tracer is not defined
| @@ -178,4 +183,22 @@ class FullStackTest < ActionDispatch::IntegrationTest | |||
| assert_equal(request_span.get_tag('http.status_code'), '404') | |||
| assert_equal(request_span.status, 0) | |||
| end | |||
|
|
|||
| test 'rails configuration enables distributed tracing' do | |||
There was a problem hiding this comment.
The change in the configure() method has been done to make this test. Mostly my intention was to be sure we can do distributed tracing using Rails settings. Since we may not need this test later (Rack will be configured separately maybe?), we may rollback the last change in the future.
palazzem
force-pushed
the
palazzem/rails-railtie
branch
from
71abec7
to
96261aa
Compare
palazzem
force-pushed
the
palazzem/rails-railtie
branch
2 times, most recently
from
5588ba1
to
88eca02
Compare
…ributed_tracing" This reverts commit 01e2230.
palazzem
force-pushed
the
palazzem/rails-railtie
branch
from
88eca02
to
6a2dacb
Compare
Overview
In the previous implementation we used another
initializerto delay the insert ofDatadog::Contrib::Rack::TraceMiddleware. Unfortunately this doesn't work in some cases as reported in #121. The patch adds theTraceMiddlewareimmediately, right before theExceptionMiddleware.Users impact
The impact may be huge so we should do proper testing before merging this PR. Tests are not enough since the problem is how the application is initialized.