[rack] add Rack middleware to trace generic Rack applications #111

palazzem · 2017-04-13T23:08:07Z

What it does

Adds a Rack middleware so that it can be used in generic Rack applications to trace the full request. This middleware can be used in applications such as Rails, so that calls are traced despite a Rails controller is used or not.

A secondary middleware will be added so that it's possible to keep track of the time spent in Rack middlewares before hitting the real application.

ufoot

2 nitpicks but LGTM

ufoot · 2017-04-14T08:45:10Z

gemfiles/contrib.gemfile

@@ -3,13 +3,13 @@
 source "https://rubygems.org"

 gem "elasticsearch-transport"
+gem "rack"


I'm surprised contrib_old.gemfile does not show up in the list of updated files. I suspect running appraisal with some older Ruby (say 1.9) should generate it.

fixed here: https://github.com/DataDog/dd-trace-rb/pull/111/files#diff-d01d3f7718f39d26d16d81a980e289eeR8

ufoot · 2017-04-14T08:53:40Z

lib/ddtrace/contrib/rack/middlewares.rb

+          # representation for this trace, so we attach the span in the Rack env
+          # so that the underlying Rack App can provide and set more details
+          request_span = @tracer.trace('rack.request', service: @service, span_type: Datadog::Ext::HTTP::TYPE)
+          request.env[:datadog_request_span] = request_span


Nitpicking, but I'm hesitating with something like :datadog_rack_request_span, even more explicit. Because here, it's obvious it's a Rack request. But, within a framework (eg Rails) that is accessing this afterwards to update it, request has also a local meaning (the rails request). So in the end you have the Rails request updating the meta (resource) for the Rack request, and in the long run, I'd say having rack in the name makes it obvious it belongs to the Rack world. But then, :datadog_rack_request_span is 26 chars so somewhat long ;) I let you decide on this, just a thought.

…ice at initialization time

palazzem · 2017-04-19T10:00:40Z

lib/ddtrace/contrib/rack/middlewares.rb

+      class TraceMiddleware
+        DEFAULT_CONFIG = {
+          tracer: Datadog.tracer,
+          default_service: 'rack'


usually we're relying in the more settings to set other things like the hostname or the port. Here I think it's better to stay clean. This is a low level instrumentation and the "traditional" way to use it, is together with another instrumentation (i.e. a library or a framework like Sinatra/Rails). If we provide a hostname here, it means that the tracer is configured twice and I don't like that, at some point, duplicated configuration can have weird side effects based on the initialization order (who configures the tracer first?).

As explained in the docs, if you want to use this middleware with custom configs, just use the tracer.configure() method before adding the middleware.

Agree we should not overdo it. As in most cases this is probably handled by the outstanding framework (Rails, Sinatra...) this should stay very simple.

palazzem · 2017-04-19T10:04:16Z

docs/GettingStarted.md

+* [Ruby on Rails](#label-Ruby+on+Rails)
+* [Sidekiq](#label-Sidekiq)
+* [Sinatra](#label-Sinatra)
+* [Rack](#label-Rack)


changed the order so that Web frameworks are at the top of the list; if you prefer I can revert the order to an alphabetical one

ufoot

Still a little issue concerning error handling I think, otherwise, amazing work, this should improve Rails instrumentation a lot.

ufoot · 2017-04-19T11:32:20Z

lib/ddtrace/contrib/rack/middlewares.rb

+      class TraceMiddleware
+        DEFAULT_CONFIG = {
+          tracer: Datadog.tracer,
+          default_service: 'rack'


Agree we should not overdo it. As in most cases this is probably handled by the outstanding framework (Rails, Sinatra...) this should stay very simple.

ufoot · 2017-04-19T11:33:38Z

lib/ddtrace/contrib/rack/middlewares.rb

+          @tracer.set_service_info(
+            @service,
+            'rack',
+            Datadog::Ext::AppTypes::WEB


Side note, wondering if, at some point we could have something like a Proxy type. It's a generic pattern, after all, just, handling & routing things for the servers doing the real job. But just a side note, this is very fine for now.

ufoot · 2017-04-19T11:37:39Z

lib/ddtrace/contrib/rack/middlewares.rb

+            resource: nil,
+            span_type: Datadog::Ext::HTTP::TYPE
+          )
+          request.env[:datadog_rack_request_span] = request_span


ufoot · 2017-04-19T11:43:54Z

lib/ddtrace/contrib/rack/middlewares.rb

+          request_span.resource = "#{request.request_method} #{status}".strip unless request_span.resource
+          request_span.set_tag('http.method', request.request_method) if request_span.get_tag('http.method').nil?
+          request_span.set_tag('http.url', request.path_info) if request_span.get_tag('http.url').nil?
+          request_span.set_tag('http.status_code', status) if request_span.get_tag('http.status_code').nil? && status


Here I think we want to catch 5xx errors (something like https://github.com/DataDog/dd-trace-rb/blob/master/lib/ddtrace/contrib/rails/action_controller.rb#L56 maybe). Note that there's always a difference between "being a Ruby exception and being an error span" (#103 shows a different case, but it illustrates both are somewhat independent).

handled here: https://github.com/DataDog/dd-trace-rb/pull/111/files#diff-dd8927189f7f5fc55f1ef3c4e46c4788R71

ufoot · 2017-04-19T11:45:45Z

test/contrib/rack/helpers.rb

+      map '/app/' do
+        run(proc do |env|
+          # this should be considered a web framework that can alter
+          # the request span after routing / controller processing


Thanks for deep testing all this, thumbs up 👍 !

ufoot · 2017-04-19T11:47:04Z

test/contrib/rack/middleware_test.rb

+    assert_equal('GET', span.get_tag('http.method'))
+    assert_equal('404', span.get_tag('http.status_code'))
+    assert_equal('/not/exists/', span.get_tag('http.url'))
+    assert_equal(0, span.status)


ufoot · 2017-04-19T11:49:27Z

test/contrib/rack/middleware_test.rb

+    assert_equal(middleware.instance_eval { @tracer }, tracer)
+    assert_equal(middleware.instance_eval { @service }, 'custom-rack')
+  end
+end


Would probably add a test with a 500 error (typically some handling that would return a 5xx but not necessarily yield an exception, those corner cases always bite us at some point) just to make clear how it behaves in such a case.

added here: https://github.com/DataDog/dd-trace-rb/pull/111/files#diff-d82f23578fe410ae54ba163846ab8d62R151

ufoot

GTM!

ufoot · 2017-04-19T12:32:25Z

lib/ddtrace/contrib/rack/middlewares.rb

+            request_span.status = 1
+            # in any case we don't touch the stacktrace if it has been set
+            if request_span.get_tag(Datadog::Ext::Errors::STACK).nil?
+              request_span.set_tag(Datadog::Ext::Errors::STACK, caller().join("\n"))


I don't think caller is of much use here, this will only give the callstack up to this point which won't even include a single line of code that has been executed down the line in the middleware. I think that info is misleading, it's probably better to not have anything here.

yes we were deciding to remove that even from Rails but since this is already a major change, we always prefer to be consistent rather than breaking the current behavior of the library. Anyway this is in the list and we'll provide an update when new changes are settled.

ufoot · 2017-04-19T12:33:10Z

test/contrib/rack/middleware_test.rb

+    assert_equal('GET', span.get_tag('http.method'))
+    assert_equal('500', span.get_tag('http.status_code'))
+    assert_equal('/app/500/no_status/', span.get_tag('http.url'))
+    assert_equal(1, span.status)


ferdynton · 2017-04-24T15:49:10Z

lib/ddtrace/contrib/rack/middlewares.rb

+
+          # call the rest of the stack
+          status, headers, response = @app.call(env)
+        rescue StandardError => e


It might have been better to use Exception => e since it's higher up in the hierarchy than StandardError, to make sure this catches as much as possible.

Actually we're not using Exception in other parts so we kept that just to be consistent. We may update that later if a higher level of Exception hierarchy must be caught. Thanks!

ferdynton · 2017-04-24T15:55:09Z

test/contrib/rack/helpers.rb

+          # this should be considered a web framework that can alter
+          # the request span after routing / controller processing
+          request_span = env[:datadog_rack_request_span]
+          request_span.set_tag('error.stack', 'Handled exception')


Since this test is about the framework not having marked the trace as status = 1, I would have also not have set the stacktrace to see how the code from the middleware handles that.

the test is about checking this code path when an inconsistent behavior is executed (setting the stacktrace but not the status). We may add a new test anyway! thanks for clarifying that!

palazzem added the integrations Involves tracing integrations label Apr 13, 2017

palazzem added this to the 0.7.0 milestone Apr 13, 2017

palazzem requested a review from ufoot April 13, 2017 23:08

palazzem mentioned this pull request Apr 13, 2017

Trace exceptions through Rack middleware #110

Closed

palazzem force-pushed the palazzem/rack-instrumentation branch from 6f11857 to b8886da Compare April 13, 2017 23:26

ufoot approved these changes Apr 14, 2017

View reviewed changes

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

Emanuele Palazzetti added 10 commits April 19, 2017 11:22

[rack] add rack-test and rack dependencies

97649a6

[rack] add implementation of a TraceMiddleware to trace a Rack request

4daf30a

[rack] test middleware constructor with defaults and custom values

c5edfc1

[rack] moving rack-test to the contrib module for compatibility reasons

bdd6666

[rack] use MiniTest for rack tests

7695955

[rack] dependencies on contrib_old.gemfile

4120b27

[rack] the Rack middleware has better defaults; configuring Rack serv…

04060d8

…ice at initialization time

[rack] add reasonable default for rack request span resource

ea78147

[rack] better namings for datadog_rack_request_span

a4a1fd7

[rack] add documentation to use the Rack middleware

315548e

palazzem force-pushed the palazzem/rack-instrumentation branch from 86bb860 to 315548e Compare April 19, 2017 09:57

palazzem changed the title ~~[rack] add Rack middlewares to trace generic Rack applications~~ [rack] add Rack middleware to trace generic Rack applications Apr 19, 2017

palazzem commented Apr 19, 2017

View reviewed changes

LotharSee approved these changes Apr 19, 2017

View reviewed changes

ufoot reviewed Apr 19, 2017

View reviewed changes

palazzem force-pushed the palazzem/rack-instrumentation branch from 96397de to ea0d4c0 Compare April 19, 2017 12:22

[rack] handle 5xx errors if not traced in the underlying framework

ba6686b

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

ufoot approved these changes Apr 19, 2017

View reviewed changes

palazzem merged commit 9b31a5b into master Apr 19, 2017

palazzem deleted the palazzem/rack-instrumentation branch April 20, 2017 16:51

ferdynton reviewed Apr 24, 2017

View reviewed changes

ferdynton mentioned this pull request Apr 25, 2017

Capture StandardError lets some Exceptions through #120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rack] add Rack middleware to trace generic Rack applications #111

[rack] add Rack middleware to trace generic Rack applications #111

palazzem commented Apr 13, 2017

ufoot left a comment

ufoot Apr 14, 2017

palazzem Apr 19, 2017

ufoot Apr 14, 2017

palazzem Apr 19, 2017 •

edited

Loading

ufoot Apr 19, 2017

palazzem Apr 19, 2017

ufoot left a comment

ufoot Apr 19, 2017

ufoot Apr 19, 2017

ufoot Apr 19, 2017

ufoot Apr 19, 2017

palazzem Apr 19, 2017

ufoot Apr 19, 2017

ufoot Apr 19, 2017

ufoot Apr 19, 2017

palazzem Apr 19, 2017

ufoot left a comment

ufoot Apr 19, 2017

ferdynton Apr 24, 2017

palazzem Apr 24, 2017

ufoot Apr 19, 2017

ferdynton Apr 24, 2017

palazzem Apr 24, 2017

ferdynton Apr 24, 2017

palazzem Apr 24, 2017

[rack] add Rack middleware to trace generic Rack applications #111

[rack] add Rack middleware to trace generic Rack applications #111

Conversation

palazzem commented Apr 13, 2017

What it does

ufoot 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

palazzem Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ufoot 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

ufoot 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

palazzem Apr 19, 2017 •

edited

Loading