Merge pull request #855 from newrelic/skip_span_event_when_transactio…

…n_ignored Do not record span events when transaction is ignored
newrelic · Nov 17, 2021 · dca8020 · dca8020
2 parents d589560 + 16e42ef
commit dca8020
Show file tree

Hide file tree

Showing 15 changed files with 148 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@
 
     Template rendering using [Tilt](https://github.com/rtomayko/tilt) is now instrumented. See [PR #847](https://github.com/newrelic/newrelic-ruby-agent/pull/847) for details.
 
+  * **Bugfix: Span Events recorded when using newrelic_ignore**
+
+    Previously, the agent was incorrectly recording span events only on transactions that should be ignored. This fix will prevent any span events from being created for transactions using newrelic_ignore, or ignored through the `rules.ignore_url_regexes` configuration option.
+
   * **Bugfix: Scrub non-unicode characters from DecoratingLogger**
 
     To prevent `JSON::GeneratorErrors`, the DecoratingLogger replaces non-unicode characters with the replacement character: �. Thank you Jonathan del Strother (@jdelStrother) for bringing this to our attention!

diff --git a/infinite_tracing/lib/infinite_tracing/agent_integrations/datastore_segment.rb b/infinite_tracing/lib/infinite_tracing/agent_integrations/datastore_segment.rb
@@ -8,6 +8,9 @@ module Agent
     class Transaction
       class DatastoreSegment
         def record_span_event
+          # don't record a span event if the transaction is ignored
+          return if transaction.ignore?
+
           tracer = ::NewRelic::Agent.agent.infinite_tracer
           tracer << Proc.new { SpanEventPrimitive.for_datastore_segment self }
         end

diff --git a/infinite_tracing/lib/infinite_tracing/agent_integrations/external_request_segment.rb b/infinite_tracing/lib/infinite_tracing/agent_integrations/external_request_segment.rb
@@ -8,6 +8,9 @@ module Agent
     class Transaction
       class ExternalRequestSegment
         def record_span_event
+          # don't record a span event if the transaction is ignored
+          return if transaction.ignore?
+
           tracer = ::NewRelic::Agent.agent.infinite_tracer
           tracer << Proc.new { SpanEventPrimitive.for_external_request_segment self }
         end

diff --git a/infinite_tracing/lib/infinite_tracing/agent_integrations/segment.rb b/infinite_tracing/lib/infinite_tracing/agent_integrations/segment.rb
@@ -13,6 +13,9 @@ def segment_complete
         end
 
         def record_span_event
+          # don't record a span event if the transaction is ignored
+          return if transaction.ignore?
+
           tracer = ::NewRelic::Agent.agent.infinite_tracer
           tracer << Proc.new { SpanEventPrimitive.for_segment self }
         end

diff --git a/infinite_tracing/test/infinite_tracing/agent_integrations/datastore_segment_test.rb b/infinite_tracing/test/infinite_tracing/agent_integrations/datastore_segment_test.rb
@@ -96,6 +96,25 @@ def test_non_sampled_segment_does_record_span_event
           assert_equal 2, span_events.size
         end
 
+        def test_ignored_transaction_does_not_record_span_event
+          span_events = generate_and_stream_segments do
+            in_web_transaction('wat') do |txn|
+              txn.stubs(:ignore?).returns(true)
+
+              segment = Tracer.start_datastore_segment(
+                product: "SQLite",
+                operation: "select",
+                port_path_or_id: 1337807
+              )
+
+              segment.start
+              advance_process_time(1.0)
+              segment.finish
+            end
+          end
+
+          assert_equal 0, span_events.size
+        end
       end
     end
   end

diff --git a/infinite_tracing/test/infinite_tracing/agent_integrations/external_request_segment_test.rb b/infinite_tracing/test/infinite_tracing/agent_integrations/external_request_segment_test.rb
@@ -86,6 +86,25 @@ def test_non_sampled_segment_does_record_span_event
           assert_equal 2, span_events.size
         end
 
+        def test_ignored_transaction_does_not_record_span_event
+          span_events = generate_and_stream_segments do
+            in_transaction('wat') do |txn|
+              txn.stubs(:ignore?).returns(true)
+
+              segment = Transaction::ExternalRequestSegment.new \
+                "Typhoeus",
+                 "http://remotehost.com/blogs/index",
+                 "GET"
+
+              txn.add_segment segment
+              segment.start
+              advance_process_time(1.0)
+              segment.finish
+            end
+          end
+
+          assert_equal 0, span_events.size
+        end
       end
     end
   end

diff --git a/infinite_tracing/test/infinite_tracing/agent_integrations/segment_test.rb b/infinite_tracing/test/infinite_tracing/agent_integrations/segment_test.rb
@@ -72,6 +72,22 @@ def test_non_sampled_segment_does_record_span_event
           assert_equal 2, span_events.size
         end
 
+        def test_ignored_transaction_does_not_record_span_event
+          span_events = generate_and_stream_segments do
+            in_transaction('wat') do |txn|
+              txn.stubs(:ignore?).returns(true)
+
+              segment = Transaction::Segment.new 'Ummm'
+              txn.add_segment segment
+              segment.start
+              advance_process_time(1.0)
+              segment.finish
+            end
+          end
+
+          assert_equal 0, span_events.size
+        end
+
         def test_streams_multiple_segments
           total_spans = 5
           segments = []

diff --git a/lib/new_relic/agent/configuration/default_source.rb b/lib/new_relic/agent/configuration/default_source.rb
@@ -1814,10 +1814,7 @@ def self.enforce_fallback(allowed_values: nil, fallback: nil)
           :type         => Array,
           :allowed_from_server => true,
           :transform    => DefaultSource.method(:convert_to_regexp_list),
-          :description  => 'Define transactions you want the agent to ignore, by specifying a list of patterns matching the URI you want to ignore.' \
-            '*Note:* This will only ignore transaction events, not spans or traces from the same transation. See documentation on ' \
-            '(Ignoring Specific Transactions)[https://docs.newrelic.com/docs/agents/ruby-agent/api-guides/ignoring-specific-transactions/#config-ignoring] ' \
-            'for more details.'
+          :description  => 'Define transactions you want the agent to ignore, by specifying a list of patterns matching the URI you want to ignore.' 
         },
         :'synthetics.traces_limit' => {
           :default      => 20,

diff --git a/lib/new_relic/agent/transaction/datastore_segment.rb b/lib/new_relic/agent/transaction/datastore_segment.rb
@@ -138,6 +138,9 @@ def record_sql?
         end
 
         def record_span_event
+          # don't record a span event if the transaction is ignored
+          return if transaction.ignore?
+
           aggregator = ::NewRelic::Agent.agent.span_event_aggregator
           priority   = transaction.priority
 

diff --git a/lib/new_relic/agent/transaction/external_request_segment.rb b/lib/new_relic/agent/transaction/external_request_segment.rb
@@ -250,6 +250,9 @@ def obfuscator
         end
 
         def record_span_event
+          # don't record a span event if the transaction is ignored
+          return if transaction.ignore?
+
           aggregator = ::NewRelic::Agent.agent.span_event_aggregator
           priority   = transaction.priority
           aggregator.record(priority: priority) do

diff --git a/lib/new_relic/agent/transaction/segment.rb b/lib/new_relic/agent/transaction/segment.rb
@@ -79,6 +79,9 @@ def segment_complete
         end
 
         def record_span_event
+          # don't record a span event if the transaction is ignored
+          return if transaction.ignore?
+
           aggregator = ::NewRelic::Agent.agent.span_event_aggregator
           priority   = transaction.priority
 

diff --git a/test/multiverse/suites/rails/ignore_test.rb b/test/multiverse/suites/rails/ignore_test.rb
@@ -17,6 +17,10 @@ def action_to_ignore
   def action_to_ignore_apdex
     render body:  "This too"
   end
+
+  def action_not_ignored
+    render body:  "Not this!"
+  end
 end
 
 class ParentController < ApplicationController
@@ -77,4 +81,20 @@ def test_apdex_ignored_if_ignored_in_parent_class
 
     assert_metrics_not_recorded("Apdex")
   end
+
+  def test_ignored_transaction_does_not_record_span_events
+    get '/ignored/action_to_ignore'
+
+    last_span_events = NewRelic::Agent.agent.span_event_aggregator.harvest![1]
+    assert_empty last_span_events
+  end
+
+  def test_ignored_by_ignore_url_regexes_does_not_record_span_events
+    with_config(:rules => { :ignore_url_regexes => ['/ignored/action_not_ignored'] }) do
+      get '/ignored/action_not_ignored'
+
+      last_span_events = NewRelic::Agent.agent.span_event_aggregator.harvest![1]
+      assert_empty last_span_events
+    end
+  end
 end
diff --git a/test/new_relic/agent/transaction/datastore_segment_test.rb b/test/new_relic/agent/transaction/datastore_segment_test.rb
@@ -210,6 +210,25 @@ def test_non_sampled_segment_does_not_record_span_event
           assert_empty last_span_events
         end
 
+        def test_ignored_transaction_does_not_record_span_event
+          in_web_transaction('wat') do |txn|
+            txn.stubs(:ignore?).returns(true)
+
+            segment = Tracer.start_datastore_segment(
+              product: "SQLite",
+              operation: "select",
+              port_path_or_id: 1337807
+            )
+
+            segment.start
+            advance_process_time 1.0
+            segment.finish
+          end
+
+          last_span_events = NewRelic::Agent.agent.span_event_aggregator.harvest![1]
+          assert_empty last_span_events
+        end
+
         def test_sampled_segment_records_span_event
           trace_id      = nil
           txn_guid      = nil

diff --git a/test/new_relic/agent/transaction/external_request_segment_test.rb b/test/new_relic/agent/transaction/external_request_segment_test.rb
@@ -877,6 +877,23 @@ def test_non_sampled_segment_does_not_record_span_event
         assert_empty last_span_events
       end
 
+      def test_non_sampled_segment_does_not_record_span_event
+        in_transaction('wat') do |txn|
+          txn.stubs(:ignore?).returns(true)
+
+          segment = ExternalRequestSegment.new "Typhoeus",
+                                               "http://remotehost.com/blogs/index",
+                                               "GET"
+          txn.add_segment segment
+          segment.start
+          advance_process_time 1.0
+          segment.finish
+        end
+
+        last_span_events = NewRelic::Agent.agent.span_event_aggregator.harvest![1]
+        assert_empty last_span_events
+      end
+
       def test_span_event_truncates_long_value
         with_config(distributed_tracing_config) do
           in_transaction('wat') do |txn|

diff --git a/test/new_relic/agent/transaction/segment_test.rb b/test/new_relic/agent/transaction/segment_test.rb
@@ -213,6 +213,21 @@ def test_non_sampled_segment_does_not_record_span_event
           assert_empty last_span_events
         end
 
+        def test_ignored_transaction_does_not_record_span_events
+          in_transaction('wat') do |txn|
+            txn.stubs(:ignore?).returns(true)
+
+            segment = Segment.new 'Ummm'
+            txn.add_segment segment
+            segment.start
+            advance_process_time 1.0
+            segment.finish
+          end
+
+          last_span_events = NewRelic::Agent.agent.span_event_aggregator.harvest![1]
+          assert_empty last_span_events
+        end
+
         def test_sampled_segment_records_span_event
           trace_id  = nil
           txn_guid  = nil