Add tests to verify active_span can be tagged from within controller methods #439

pawelchcki · 2018-05-30T14:36:18Z

While implementing possible alternative solution to #436
I've found that we already support tagging spans from within Controller methods.

This PR is meant to salvage some of that work

Todo:

add documentation on how to access span from within nested method

…for setting custom tags

delner · 2018-05-30T16:54:41Z

lib/ddtrace/contrib/rails/core_extensions.rb

    # ActionController patch for Ruby 2.0+
    module ActionControllerPatch
+      # compat for Ruby versions not supporting #prepend
+      include CompatActionControllerPatch unless Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.0.0')


How does this line work? Won't doing an include here only include it into the ActionControllerPatch, and not into the ActionController::Instrumentation module? Typically the only way to nest module inheritance is to implement the def self.included(base) method.

Its here to build proper ancestry chain for ActionControllerPatch to work when included in Ruby < 2.0.

delner · 2018-05-30T17:06:25Z

lib/ddtrace/contrib/rails/core_extensions.rb

-            alias_method :process_action, :process_action_with_datadog
+            remove_method :process_action
+
+            include ActionControllerPatch


I don't think you can just include the patch here; this is something I spent a lot of time trying to figure out. I think you need prepend in order to inject the patch at the top of the ancestor stack.

prepend is only required to override existing methods. In ruby < 2.0 we can still do that but it requires some additional steps.

This is to make sure we reuse the same class and instrumentation implementation for both versions. Ensuring there are no difference in actual instrumentation implementation when running ruby 1.9.3.

delner · 2018-05-30T17:10:00Z

lib/ddtrace/contrib/rails/core_extensions.rb

@@ -163,49 +163,36 @@ def patch_process_action
        else
          # Rewrite module that gets composed into the Rails controller base class
          ::ActionController::Instrumentation.class_eval do
-            def process_action_with_datadog(*args)


I think it's great that we're adding tests to cover a scenario we didn't test previously. But I don't think it's a good time to make big changes to how this works. The implementation might be a bit ugly, but the added benefit of making it a little cleaner vs the risk of introducing bugs is not a good trade. In the longer term, we'd like to drop support for 1.9 altogether, so we'll just simply remove this code instead of refactoring it.

I don't see it as big change. Code deduplication here is used to make sure there are no differences in behavior when running on ruby 1.9.3 or 2.00.

I touched this code, because before I found another way, I thought that I need to add some additional behavior to both methods. Instead of duplicating code I decided to ensure both implementations will allways be the same. But since that went away, I still left the change as in my opinion it improves the existing code.

AFAIK there is no timeline for deprecating 1.9.3 support so at this moment, I think its still worthwhile to treat 1.9.3 as first class citizen.

…ed methods

delner

Just one suggestion to make the patch changes you've introduced a little more idiomatic, and some comments about documentation changes.

delner · 2018-05-31T16:44:07Z

docs/GettingStarted.md

+  def index
+    return head :not_found unless authenticated?
+
+    tracer = Datadog.configuration[:rails][:tracer]


Although this is a technically correct example, it applies very specifically to Rails. Would it be better to provide a more generic example? Don't want users to be hung up on how to get the tracer. Maybe something like:

current_span = Datadog.tracer.active_span unless current_span.nil? current_span.set_tag('my_tag', 'my_value') end

Yeah, I was quite curious which way should be cannonical. Since user can set different tracer for each integration.

Datadog.tracer.active_span looks much nicer - lets use it.

delner · 2018-05-31T16:46:14Z

docs/GettingStarted.md

+    return head :not_found unless authenticated?
+
+    tracer = Datadog.configuration[:rails][:tracer]
+    tracer.active_span&.set_tag('authenticated', true)


I don't think we should use the &. operator because we support 1.9.3/2.0.0/2.1.10/2.2.10 right now, and those versions don't have the safe operator. (Don't want to cause confusion with users who might copy-paste code.)

Yeah since we support the old versions, I hoped I would be able to use this operator in the docs at least. But you're right, that less surprises for our users == :better.

delner · 2018-05-31T17:01:34Z

lib/ddtrace/contrib/rails/core_extensions.rb

+
+    # ActionController patch
+    module ActionControllerPatch
+      def self.instrument(klass)


The strategy here is good, but it isn't quite as idiomatic as it could be. I might suggest restructuring this a bit to take advantage of the Ruby included method to drive this. Something like:

module ActionControllerPatch def self.included(base) if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.0.0') base.send(:prepend, ProcessActionPatch) else base.class_eval do alias_method :process_action_without_datadog, :process_action include ProcessActionPatch end end end module ProcessActionCompatibilityPatch def process_action(*args) process_action_without_datadog(*args) end end module ProcessActionPatch # compat for Ruby versions not supporting #prepend include ProcessActionCompatibilityPatch unless Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.0.0') def process_action(*args) # ... end end end

This would enable you to simply do ::ActionController::Metal.send(:include, ActionControllerPatch) to apply the patch.

Some benefits of doing so:

Looks a bit more idiomatic

You effectively prevent patching the controller twice, by using include instead of a generic module function.

You can compartmentalize all of the modules needed to patch ActionController within the ActionControllerPatch module.

delner · 2018-05-31T17:05:34Z

lib/ddtrace/contrib/rails/core_extensions.rb

+    end
+
+    # Compat module for Ruby versions not supporting #prepend
+    module CompatActionControllerPatch


Compat should be expanded into Compatible or Compatibility for readability.

…g.tracer` API

- backported fix for status being Array into refactored patcher # Conflicts: # lib/ddtrace/contrib/rails/core_extensions.rb

delner

I'm happy with the code changes, although I still think we should change the documentation.

delner · 2018-06-04T20:13:43Z

docs/GettingStarted.md

+
+You can tag additional information to current active span from any method. Note however that if the method is called and there is no span currently active `active_span` will be nil. 
+
+```ruby


I still think this example should be more generalized; don't want users to think this only applies to Rails controllers. See my comment from my previous review.

Good point! Fixed

delner · 2018-06-04T20:13:58Z

lib/ddtrace/contrib/rails/action_controller_patch.rb

+    module Rails
+      # Instrument ActiveController processing
+      module ActionControllerPatch
+        def self.included(base)


pawelchcki · 2018-06-05T11:13:12Z

@delner addressed the review comments.

delner

👍 Looks good! Thanks @pawelchcki !

pawelchcki added do-not-merge/WIP Not ready for merge dev/refactor Involves refactoring existing components labels May 30, 2018

Use only single class implementing ActiveController patch + add test …

e6ac666

…for setting custom tags

pawelchcki force-pushed the feature/add_ability_to_set_access_current_span_from_within_controller branch from e28fb51 to e6ac666 Compare May 30, 2018 14:38

remove unnecessary whitespace

98c16c4

delner reviewed May 30, 2018

View reviewed changes

Overcome problem with ancestry

6ea9366

pawelchcki force-pushed the feature/add_ability_to_set_access_current_span_from_within_controller branch from bd7cb21 to 6ea9366 Compare May 31, 2018 10:45

Add small documentation snippet explaining how to tag spans from nest…

a6e63da

…ed methods

pawelchcki added docs Involves documentation and removed do-not-merge/WIP Not ready for merge labels May 31, 2018

pawelchcki assigned delner May 31, 2018

delner reviewed May 31, 2018

View reviewed changes

delner assigned pawelchcki and unassigned delner May 31, 2018

pawelchcki added 4 commits June 4, 2018 13:35

Make docs example more compatible with older versions and use `Datado…

5c48691

…g.tracer` API

Refactor ActionControllerPatch to be more ruby idiomatic

2baafbd

Merge remote-tracking branch 'origin/master' into HEAD

dcaf074

- backported fix for status being Array into refactored patcher # Conflicts: # lib/ddtrace/contrib/rails/core_extensions.rb

Rubocop formatting

a4a084d

pawelchcki force-pushed the feature/add_ability_to_set_access_current_span_from_within_controller branch from ac2e71a to a4a084d Compare June 4, 2018 11:59

Fix for ruby 2.0 and 1.9

69c1106

delner added the integrations Involves tracing integrations label Jun 4, 2018

delner requested changes Jun 4, 2018

View reviewed changes

make the example non rails specifc

b993627

pawelchcki assigned delner and unassigned pawelchcki Jun 5, 2018

delner approved these changes Jun 5, 2018

View reviewed changes

pawelchcki merged commit 0b2ca24 into master Jun 5, 2018

pawelchcki deleted the feature/add_ability_to_set_access_current_span_from_within_controller branch June 5, 2018 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests to verify active_span can be tagged from within controller methods #439

Add tests to verify active_span can be tagged from within controller methods #439

pawelchcki commented May 30, 2018 •

edited

delner May 30, 2018

pawelchcki May 31, 2018

delner May 30, 2018

pawelchcki May 31, 2018

delner May 30, 2018

pawelchcki May 31, 2018

delner left a comment

delner May 31, 2018

pawelchcki Jun 4, 2018

delner May 31, 2018

pawelchcki Jun 4, 2018

delner May 31, 2018

pawelchcki Jun 4, 2018

delner May 31, 2018

pawelchcki Jun 4, 2018

delner left a comment

delner Jun 4, 2018

pawelchcki Jun 5, 2018

delner Jun 4, 2018

pawelchcki commented Jun 5, 2018

delner left a comment


		You can tag additional information to current active span from any method. Note however that if the method is called and there is no span currently active `active_span` will be nil.

		```ruby

Add tests to verify active_span can be tagged from within controller methods #439

Add tests to verify active_span can be tagged from within controller methods #439

Conversation

pawelchcki commented May 30, 2018 • edited

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 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

delner 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

pawelchcki commented Jun 5, 2018

delner left a comment

Choose a reason for hiding this comment

pawelchcki commented May 30, 2018 •

edited