Drop spans for child-to-parent ordered traces in SpanFilter

lib/datadog/tracing/pipeline/span_filter.rb

@@ -15,17 +15,21 @@ module Pipeline
       #
       # @public_api
       class SpanFilter < SpanProcessor
-        # NOTE: this SpanFilter implementation only handles traces in which child spans appear
-        # after parent spans in the trace array. If in the future child spans can be before
+        # NOTE: This SpanFilter implementation only handles traces in which child spans appear
+        # before parent spans in the trace array. If in the future child spans can be after
         # parent spans, then the code below will need to be updated.
         # @!visibility private
         def call(trace)
           deleted = Set.new
 
-          trace.spans.delete_if do |span|
+          span_count = trace.spans.length
+          trace.spans.reverse_each.with_index do |span, i|
             should_delete = deleted.include?(span.parent_id) || drop_it?(span)
-            deleted << span.id if should_delete
-            should_delete
+
+            if should_delete
+              deleted << span.id
+              trace.spans.delete_at(span_count - 1 - i)
+            end
           end
 
           trace

spec/datadog/tracing/pipeline/span_filter_spec.rb

@@ -9,7 +9,8 @@
 RSpec.describe Datadog::Tracing::Pipeline::SpanFilter do
   include PipelineHelpers
 
-  let(:trace) { Datadog::Tracing::TraceSegment.new([span_a, span_b, span_c]) }
+  let(:trace) { Datadog::Tracing::TraceSegment.new(trace_spans) }
+  let(:trace_spans) { [span_a, span_b, span_c] }
   let(:span_a) { generate_span('a') }
   let(:span_b) { generate_span('b') }
   let(:span_c) { generate_span('c') }
@@ -35,28 +36,54 @@
     end
 
     context 'with spans that have a parent' do
-      let(:trace) { Datadog::Tracing::TraceSegment.new([span_a, span_b, span_c, span_d]) }
-
       let(:filter_regex) { /a/ }
       let(:span_b) { generate_span('b', span_a) }
       let(:span_c) { generate_span('c', span_b) }
       let(:span_d) { generate_span('d') }
 
-      it 'filters out any child spans of a span that matches the criteria' do
-        expect { span_filter.call(trace) }
-          .to change { trace.spans }
-          .from([span_a, span_b, span_c, span_d])
-          .to([span_d])
+      context 'in grandchild-to-grandparent order' do
+        let(:trace_spans) { [span_d, span_c, span_b, span_a] }
+
+        it 'filters out any child spans of a span that matches the criteria' do
+          expect { span_filter.call(trace) }
+            .to change { trace.spans }
+            .from(trace_spans)
+            .to([span_d])
+        end
+
+        context 'with spans that have a parent span that doesnt match filtering criteria' do
+          let(:filter_regex) { /b/ }
+
+          it 'does not filter out parent spans of child spans that matches the criteria' do
+            expect { span_filter.call(trace) }
+              .to change { trace.spans }
+              .from(trace_spans)
+              .to([span_d, span_a])
+          end
+        end
       end
 
-      context 'with spans that have a parent span that doesnt match filtering criteria' do
-        let(:filter_regex) { /b/ }
+      context 'in grandparent-to-grandchild order' do
+        let(:trace_spans) { [span_a, span_b, span_c, span_d] }
+
+        before { skip('Parent-to-child order filtering not supported.') }
 
-        it 'does not filter out parent spans of child spans that matches the criteria' do
+        it 'filters out any child spans of a span that matches the criteria' do
           expect { span_filter.call(trace) }
             .to change { trace.spans }
-            .from([span_a, span_b, span_c, span_d])
-            .to([span_a, span_d])
+            .from(trace_spans)
+            .to([span_d])
+        end
+
+        context 'with spans that have a parent span that doesnt match filtering criteria' do
+          let(:filter_regex) { /b/ }
+
+          it 'does not filter out parent spans of child spans that matches the criteria' do
+            expect { span_filter.call(trace) }
+              .to change { trace.spans }
+              .from(trace_spans)
+              .to([span_a, span_d])
+          end
         end
       end
     end