[rails] Rails action_controller is part of the rack request #112

palazzem · 2017-04-14T10:11:52Z

What it does

Major change about how Rails applications are traced. Now the Rack TraceMiddleware is used and so an high overview of the whole request is available. Changes include:

the TraceMiddleware is added at initialization time, before the middleware stack is built
the rails.request now becomes rails.action_controller and is not the root span anymore
because the TraceMiddleware is used, rack.request is the root span for all traces and it includes all http-related tags
the rails.request is now part of the rails-controller service (it's user configurable)
when we're inside a Rails controller, the process_action.action_controller() changes the rack.request resource so that it has a meaningful name
if Rails is not hit, the rack.request keeps the default resource name like GET 200 or GET 404

…are span; now it is not the root request

Even though Integration tests are said to be 25% slower, they run through Rake's middleware stack, which we might leverage in order to improve the error capturing capabilities.

ActiveSupport's instrumentation doesn't provide a lot of information about an error that might have been raised, so introducing a piece of middleware seems to be the best way of getting full control. Other similar gems also adopt this approach.

…re_stack and configures the Rack middleware

This reverts commit 751267346472001cc9beb310e4b0b9c3ffa49e8a.

… only if the parent is available

ufoot · 2017-04-14T11:36:10Z

docs/GettingStarted.md

@@ -58,8 +58,9 @@ of the Datadog tracer, you can override the following defaults:
      auto_instrument: false,
      auto_instrument_redis: false,
      default_service: 'rails-app',
-      default_database_service: 'postgresql',
+      default_controller_service: 'rails-controller',


ufoot · 2017-04-14T11:45:35Z

lib/ddtrace/contrib/rack/middlewares.rb

+            raise e
+          ensure
+            # we must close all active spans and set span fields if not already set
+            request_span.finish()


Don't we have a risk to flush/write this before the tags are set? I mean, today's code path might not make this possible, but generally speaking, I tend to think .finish() should be the last call.

ufoot · 2017-04-20T15:32:13Z

test/contrib/rails/controller_test.rb

@@ -21,12 +21,9 @@ class TracingControllerTest < ActionController::TestCase
    assert_equal(spans.length, 2)

    span = spans[1]
-    assert_equal(span.name, 'rails.request')
+    assert_equal(span.name, 'rails.action_controller')
    assert_equal(span.span_type, 'http')
    assert_equal(span.resource, 'TracingController#index')


So, as I understand it, all this http metadata has been bounced to rack, which is actually handling the protocol and should carry the now missing info in its spans, right?

yes, all http-related tags are handled in the Rack middleware. I think this is the right approach, especially because a middleware can still change the status_code when the middleware stack is executed "backward" (in the sense of code after the @app.call()).

ufoot · 2017-04-20T15:33:41Z

docs/GettingStarted.md

@@ -84,9 +85,10 @@ Available settings are:
 * ``auto_instrument_redis``: if set to ``true`` Redis calls will be traced as such. Calls to Redis cache may be
  still instrumented but you will not have the detail of low-level Redis calls.
 * ``default_service``: set the service name used when tracing application requests. Defaults to ``rails-app``
+* ``default_controller_service``: set the service name used when tracing a Rails action controller. Defaults to ``rails-controller``


I might need to refer to this in the ChangeLog/release note BTW.

yes, that's part of our "breaking change", that basically doesn't break anything, but changes the previous behavior adding this layer in the middle

…tation

palazzem added integrations Involves tracing integrations do-not-merge/WIP Not ready for merge labels Apr 14, 2017

palazzem added this to the 0.7.0 milestone Apr 14, 2017

palazzem requested a review from ufoot April 14, 2017 10:11

palazzem force-pushed the palazzem/rack-instrumentation branch from b8886da to 86bb860 Compare April 14, 2017 10:13

palazzem force-pushed the palazzem/rails-rack branch from 1f1b560 to be958e5 Compare April 14, 2017 10:15

palazzem force-pushed the palazzem/rack-instrumentation branch 2 times, most recently from ea0d4c0 to ba6686b Compare April 19, 2017 12:31

Emanuele Palazzetti and others added 8 commits April 20, 2017 17:18

[rails] the Rack middleware is used

b61ed7a

[rails] the active_controller instrumentation configures rack middlew…

640d6ab

…are span; now it is not the root request

[rails] convert Rails Controller tests into Integration ones

96b525e

Even though Integration tests are said to be 25% slower, they run through Rake's middleware stack, which we might leverage in order to improve the error capturing capabilities.

[rails] datadog_config initializer is executed before :build_middlewa…

0b62ec4

…re_stack and configures the Rack middleware

[rails] updated Rails testing suite for integration tests

721269b

[rails] keep the previous test suite as functional tests

efa4ee4

This reverts commit 751267346472001cc9beb310e4b0b9c3ffa49e8a.

[rails] update all tests for the new API; parent span resource is set…

6a5dc50

… only if the parent is available

palazzem force-pushed the palazzem/rails-rack branch from be958e5 to cb18a45 Compare April 20, 2017 15:19

palazzem changed the base branch from palazzem/rack-instrumentation to master April 20, 2017 15:19

ufoot approved these changes Apr 20, 2017

View reviewed changes

[rails] add integration tests for Rack middleware and Rails instrumen…

33f79a5

…tation

palazzem force-pushed the palazzem/rails-rack branch from cb18a45 to 33f79a5 Compare April 20, 2017 15:47

palazzem removed the do-not-merge/WIP Not ready for merge label Apr 20, 2017

[rails] updated sidekiq test

9e9be56

palazzem merged commit cf71180 into master Apr 20, 2017

palazzem deleted the palazzem/rails-rack branch April 20, 2017 19:40

palazzem mentioned this pull request Apr 24, 2017

Trace exceptions through Rack middleware #110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rails] Rails action_controller is part of the rack request #112

[rails] Rails action_controller is part of the rack request #112

palazzem commented Apr 14, 2017 •

edited

Loading

ufoot Apr 14, 2017

ufoot Apr 14, 2017

ufoot Apr 20, 2017

palazzem Apr 20, 2017

ufoot Apr 20, 2017

palazzem Apr 20, 2017

[rails] Rails action_controller is part of the rack request #112

[rails] Rails action_controller is part of the rack request #112

Conversation

palazzem commented Apr 14, 2017 • edited Loading

What it does

ufoot Apr 14, 2017

Choose a reason for hiding this comment

ufoot Apr 14, 2017

Choose a reason for hiding this comment

ufoot Apr 20, 2017

Choose a reason for hiding this comment

palazzem Apr 20, 2017

Choose a reason for hiding this comment

ufoot Apr 20, 2017

Choose a reason for hiding this comment

palazzem Apr 20, 2017

Choose a reason for hiding this comment

palazzem commented Apr 14, 2017 •

edited

Loading