Implement Sinatra Env and Headers extensions #441

delner · 2018-05-31T21:54:50Z

As a suggestion for #427, I felt inspired to try a spin on @pawelchcki 's idea.

This pull request implements a Sinatra::Env and Sinatra::Headers extension, which decorates the env passed into the middleware with some Sinatra-tracing specific features. The benefit of doing this means our utility features become available, and context specific, across all the components of our Sinatra tracing, wherever env is available.

Another potentially cool benefit is the Env and Headers modules aren't very Sinatra specific: they could be extracted and used for any Rack apps, so this might aid our efforts to undertake that refactor as well.

This PR would be intended to be merged into #427 . @pawelchcki , let me know what you think!

pawelchcki · 2018-06-04T10:19:01Z

lib/ddtrace/contrib/sinatra/tracer_middleware.rb

        def initialize(app)
          @app = app
        end

        def call(env)
-          span = RequestSpan.span!(env)
+          # Extend the Env with Sinatra tracing functions
+          env.extend(Sinatra::Env)


Interesting approach. I'm worried however that this might introduce somewhat unstable API to rack envs.

Imagine another rack middleware used in the same application. That does use env.datadog_span accessor methods. It all works well as long as the order of MWs is TracerMiddleware -> OtherMiddleware.

When the order gets reversed then calling env.datadog_span will throw NoMethodError.

As such I think it would be much safer to use module functions to share the code operating on env.
If avoiding object allocations wasn't one of our goals, I would suggest using Composition over Inheritance. Because env.extend is Ruby specific runtime inheritance.

In general also extending objects this way should be avoided due to reasons mentioned above, where order of execution change would mean the code could suddenly throw NoMethodError.

Fair concern about datadog_span, regarding the change in middleware order. However, I don't think whether you extend or call the module will make a difference anyways.

My reason for saying so is imagine this second middleware that was dependent on env.datadog_span. Instead, it will call to a function like Sinatra::Env.datadog_span(env) which won't raise a NoMethodError now, but will still fail because the span will be missing from the env because TracerMiddleware didn't run yet. In this case, OtherMiddleware still depends on TracerMiddleware no matter how you articulate it. And if there is such a dependency, it's reasonable that the dependent be responsible for placing itself in the right position (i.e. the developer should be responsible for configuring their middleware stack correctly.)

However, I will yield that extend is a bit unorthodox, and by using it, you might have inconsistency between objects of the same type; something in the general sense that might not sit well with many developers. As such, I think it is reasonable to remove the extend implementation in favor of something simpler, albeit not as elegant.

pawelchcki

Thanks @delner for creating this PR making the code more reusable.

However overall obj.extend can cause problems down the line by introducing API's that change based on order of execution. Using that in production code should be carefully weighed.

delner · 2018-06-04T15:48:55Z

@pawelchcki Thanks for the feedback!

I've altered the modules to implement their functions as 'static', instead of being extended by the Rack objects directly. Will this resolve your concerns? Feel free to take another look!

pawelchcki

Looks good, thanks for making this PR @delner!.

I've left a few minor remarks about TODOs and code comments describing what code below is doing.

One suggestion, would you mind rebasing this on 0.13-dev? So that this change would not block merging of
#427

I'm still happy to merge this once the remarks are addressed I only think we could do it after #427 is merged since this change is quite self contained.

pawelchcki · 2018-06-05T10:53:23Z

lib/ddtrace/contrib/sinatra/env.rb

+  module Contrib
+    module Sinatra
+      # Gets and sets trace information from a Rack Env
+      # TODO: Extract me?


Could we handle this TODO i.e. implement the suggestion or create Trello card to track this ?

I can remove this comment for now. We should extract this in a future PR, ideally as a part of the Rack/Sinatra header refactor PR you had opened. (#432)

I'll add that there as a TODO so we won't lose this idea

pawelchcki · 2018-06-05T10:53:47Z

lib/ddtrace/contrib/sinatra/headers.rb

+  module Contrib
+    module Sinatra
+      # Gets and sets trace information from a Rack headers Hash
+      # TODO: Extract me?


Could we handle this TODO as well ?

pawelchcki · 2018-06-05T10:54:28Z

lib/ddtrace/contrib/sinatra/tracer_middleware.rb

+        private
+
+        def tracer
+          configuration[:tracer]


pawelchcki · 2018-06-05T10:56:26Z

lib/ddtrace/contrib/sinatra/tracer_middleware.rb

+            service: configuration[:service_name],
+            span_type: Datadog::Ext::HTTP::TYPE
+          ) do |span|
+            # Set the span on the Env


Is this comment necessary? It seems to duplicate what the line below says.
Maybe we could thing about renaming the method below so that this comment is necessary ?

pawelchcki · 2018-06-05T10:56:44Z

lib/ddtrace/contrib/sinatra/tracer_middleware.rb

-        ensure
-          span.finish
-        end
+            # Tag request headers


Is this comment necessary? It seems to duplicate what the line below says.
Maybe we could thing about renaming the method below so that this comment is necessary ?

pawelchcki · 2018-06-05T10:56:56Z

lib/ddtrace/contrib/sinatra/tracer_middleware.rb

-              if env.key?(rack_header)
-                result[Datadog::Ext::HTTP::RequestHeaders.to_tag(header)] = env[rack_header]
-              end
+            # Tag response headers


Is this comment necessary? It seems to duplicate what the line below says.
Maybe we could thing about renaming the method below so that this comment is necessary ?

pawelchcki · 2018-06-05T10:59:43Z

lib/ddtrace/contrib/sinatra/tracer_middleware.rb

            end
+
+            # Return response


Since ruby has implicit returns and and I think we enforce them in Rubocop, maybe we should consider removing that restriction so you could explicitly state return [status, headers, response_body] if you feel it improves readablity

I suppose you're right. I'll remove all of these extraneous comments if you don't think it helps.

delner · 2018-06-05T14:48:36Z

@pawelchcki I think that's reasonable. We can merge this one after.

delner · 2018-06-05T15:20:23Z

@pawelchcki Fixed the comments and rebased on 0.13-dev. This one should be ready for another review.

pawelchcki · 2018-06-05T15:59:59Z

Thanks for making these changes @delner. Looks really good now! Lets merge it.

delner added integrations Involves tracing integrations dev/refactor Involves refactoring existing components labels May 31, 2018

delner self-assigned this May 31, 2018

delner requested a review from pawelchcki May 31, 2018 21:54

delner force-pushed the feature/sinatra_env_headers_extensions branch from 2018809 to d4175f2 Compare May 31, 2018 22:05

pawelchcki reviewed Jun 4, 2018

View reviewed changes

pawelchcki requested changes Jun 4, 2018

View reviewed changes

pawelchcki requested changes Jun 5, 2018

View reviewed changes

delner and others added 5 commits June 5, 2018 11:17

Added: Env and Headers modules for Sinatra.

2a977ba

Changed: Sinatra Tracer and TracerMiddleware to use Env and Headers.

0cb4f00

Removed: Sinatra::RequestSpan module.

7588393

Changed: Sinatra extensions to be called instead of inject.

18b3840

Removed: Some comments on the Sinatra tracer.

7905dd6

delner force-pushed the feature/sinatra_env_headers_extensions branch from de49de7 to 7905dd6 Compare June 5, 2018 15:18

delner changed the base branch from feature/sinatra_add_request_id to 0.13-dev June 5, 2018 15:19

pawelchcki mentioned this pull request Jun 5, 2018

Sinatra - Rack - refactoring common code #432

Closed

1 task

pawelchcki approved these changes Jun 5, 2018

View reviewed changes

delner merged commit 851ad82 into 0.13-dev Jun 5, 2018

delner deleted the feature/sinatra_env_headers_extensions branch June 5, 2018 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Sinatra Env and Headers extensions #441

Implement Sinatra Env and Headers extensions #441

delner commented May 31, 2018 •

edited

Loading

pawelchcki Jun 4, 2018

delner Jun 4, 2018 •

edited

Loading

pawelchcki left a comment

delner commented Jun 4, 2018

pawelchcki left a comment

pawelchcki Jun 5, 2018

delner Jun 5, 2018 •

edited

Loading

pawelchcki Jun 5, 2018

pawelchcki Jun 5, 2018

pawelchcki Jun 5, 2018

pawelchcki Jun 5, 2018

pawelchcki Jun 5, 2018

pawelchcki Jun 5, 2018

pawelchcki Jun 5, 2018

delner Jun 5, 2018

delner commented Jun 5, 2018

delner commented Jun 5, 2018

pawelchcki commented Jun 5, 2018

Implement Sinatra Env and Headers extensions #441

Implement Sinatra Env and Headers extensions #441

Conversation

delner commented May 31, 2018 • edited Loading

Choose a reason for hiding this comment

delner Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

pawelchcki left a comment

Choose a reason for hiding this comment

delner commented Jun 4, 2018

pawelchcki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner Jun 5, 2018 • edited Loading

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

delner commented Jun 5, 2018

delner commented Jun 5, 2018

pawelchcki commented Jun 5, 2018

delner commented May 31, 2018 •

edited

Loading

delner Jun 4, 2018 •

edited

Loading

delner Jun 5, 2018 •

edited

Loading