Merge pull request #59 from solarwindscloud/NH-52516

NH-52516: return false if span is not sampled or invalid

Rakefile

@@ -26,7 +26,8 @@ Rake::TestTask.new do |t|
 
   when 'unit.gemfile'
     t.test_files = FileList['test/unit/*_test.rb'] +
-                   FileList['test/component/*_test.rb']
+                   FileList['test/component/*_test.rb'] +
+                   FileList['test/api/*_test.rb']
   end
 end
 

lib/solarwinds_apm/api/transaction_name.rb

@@ -31,27 +31,29 @@ module TransactionName
       #
       def set_transaction_name(custom_name=nil)
 
-        return false if custom_name.nil? || custom_name.empty? 
-        return true  if SolarWindsAPM::Context.toString == '00-00000000000000000000000000000000-0000000000000000-00' # noop
+        status = true
+        if custom_name.nil? || custom_name.empty? 
+          status = false
+        elsif SolarWindsAPM::Context.toString == '99-00000000000000000000000000000000-0000000000000000-00' # noop
+          status = true
+        else
+          solarwinds_processor = SolarWindsAPM::OTelConfig.class_variable_get(:@@config)[:span_processor]
+          if solarwinds_processor.nil?
+            SolarWindsAPM.logger.warn {"[#{name}/#{__method__}] Solarwinds processor is missing. Set transaction name failed."}
+            status = false
+          else
+            entry_trace_id = ::OpenTelemetry::Baggage.value(::SolarWindsAPM::Constants::INTL_SWO_CURRENT_TRACE_ID)
+            entry_span_id  = ::OpenTelemetry::Baggage.value(::SolarWindsAPM::Constants::INTL_SWO_CURRENT_SPAN_ID)
+            trace_flags    = ::OpenTelemetry::Baggage.value(::SolarWindsAPM::Constants::INTL_SWO_CURRENT_TRACE_FLAG)
 
-        solarwinds_processor = SolarWindsAPM::OTelConfig.class_variable_get(:@@config)[:span_processor]
-        if solarwinds_processor.nil?
-          SolarWindsAPM.logger.warn {"[#{name}/#{__method__}] Solarwinds processor is missing. Set transaction name failed."}
-          return false
-        end
-
-        entry_trace_id = ::OpenTelemetry::Baggage.value(::SolarWindsAPM::Constants::INTL_SWO_CURRENT_TRACE_ID)
-        entry_span_id  = ::OpenTelemetry::Baggage.value(::SolarWindsAPM::Constants::INTL_SWO_CURRENT_SPAN_ID)
+            status = false if entry_trace_id.nil? || entry_span_id.nil? || trace_flags.nil?
+            status = false if entry_trace_id == '0'*32 || entry_span_id == '0'*16 || trace_flags == '00' # not sampled
 
-        if entry_trace_id.nil? || entry_span_id.nil? 
-          SolarWindsAPM.logger.warn {"[#{name}/#{__method__}] Cannot cache custom transaction name #{custom_name} because OTel service entry span not started; ignoring"}
-          return false
+            solarwinds_processor.txn_manager.set("#{entry_trace_id}-#{entry_span_id}",custom_name) 
+            SolarWindsAPM.logger.debug {"[#{name}/#{__method__}] Cached custom transaction name for #{entry_trace_id}-#{entry_span_id} as #{custom_name}"}
+          end
         end
-
-        trace_span_id = "#{entry_trace_id}-#{entry_span_id}"
-        solarwinds_processor.txn_manager.set(trace_span_id,custom_name) 
-        SolarWindsAPM.logger.debug {"[#{name}/#{__method__}] Cached custom transaction name for #{trace_span_id} as #{custom_name}"}
-        true
+        status
       end
     end
   end

lib/solarwinds_apm/noop/context.rb

@@ -12,7 +12,7 @@ module Context
     # toString would return the current trace context as string
     #
     def self.toString
-      '00-00000000000000000000000000000000-0000000000000000-00'
+      '99-00000000000000000000000000000000-0000000000000000-00'
     end
 
     ##

lib/solarwinds_apm/opentelemetry/solarwinds_processor.rb

@@ -147,6 +147,7 @@ def calculate_transaction_names(span)
         trace_span_id = "#{span.context.hex_trace_id}-#{span.context.hex_span_id}"
         if @txn_manager.get(trace_span_id)
           trans_name = @txn_manager.get(trace_span_id)
+          @txn_manager.del(trace_span_id)
         else
           trans_name = span.attributes[HTTP_ROUTE] || nil
           trans_name = span.name if span.name && (trans_name.nil? || trans_name.empty?)

lib/solarwinds_apm/opentelemetry/solarwinds_sampler.rb

@@ -65,9 +65,9 @@ def should_sample?(trace_id:, parent_context:, links:, name:, kind:, attributes:
         new_trace_state = calculate_trace_state(liboboe_decision,parent_span_context,xtraceoptions)
         SolarWindsAPM.logger.debug {"[#{self.class}/#{__method__}] new_trace_state: #{new_trace_state.inspect}"}
 
-        new_attributes  = calculate_attributes(attributes,liboboe_decision,new_trace_state,parent_span_context,xtraceoptions)
+        new_attributes  = otel_sampled?(otel_decision)? calculate_attributes(attributes,liboboe_decision,new_trace_state,parent_span_context,xtraceoptions) : nil
         SolarWindsAPM.logger.debug {"[#{self.class}/#{__method__}] new_attributes: #{new_attributes.inspect}"}
-        
+
         sampling_result = ::OpenTelemetry::SDK::Trace::Samplers::Result.new(decision: otel_decision, attributes: new_attributes, tracestate: new_trace_state)
         SolarWindsAPM.logger.debug {"[#{self.class}/#{__method__}] sampling_result: #{sampling_result.inspect}"}
 
@@ -234,7 +234,7 @@ def calculate_trace_state(decision, parent_span_context, xtraceoptions)
         if xtraceoptions&.options
           trace_state = trace_state.set_value(
             XTraceOptions.sw_xtraceoptions_response_key.to_s,
-            create_xtraceoptions_response_value(decision,parent_span_context,xtraceoptions))
+            create_xtraceoptions_response_value(decision, parent_span_context, xtraceoptions))
         end
 
         trace_state
@@ -285,11 +285,6 @@ def add_tracestate_capture_to_attributes_dict(attributes_dict, decision, trace_s
       def calculate_attributes(attributes, decision, trace_state, parent_span_context, xtraceoptions)
         SolarWindsAPM.logger.debug {"[#{self.class}/#{__method__}] Received attributes: #{attributes.inspect}; decision:#{decision.inspect}; trace_state:#{trace_state.inspect}; parent_span_context:#{parent_span_context.inspect}; xtraceoptions:#{xtraceoptions.inspect}"}
 
-        otel_decision = otel_decision_from_liboboe(decision)
-        return nil if Transformer.sampled?(otel_decision) == false
-
-        SolarWindsAPM.logger.debug {"[#{self.class}/#{__method__}] Trace decision is_sampled - setting attributes #{otel_decision.inspect}"}
-
         new_attributes = {}
         # Copy existing MappingProxyType KV into new_attributes for modification.
         attributes&.each {|k,v| new_attributes[k] = v}
@@ -330,6 +325,10 @@ def sw_from_span_and_decision(parent_span_context, decision)
         trace_flag = Transformer.trace_flags_from_boolean(decision["do_sample"])
         Transformer.sw_from_span_and_decision(parent_span_context.hex_span_id, trace_flag)
       end
+
+      def otel_sampled?(otel_decision)
+        otel_decision == ::OpenTelemetry::SDK::Trace::Samplers::Decision::RECORD_AND_SAMPLE
+      end
     end
   end
 end

lib/solarwinds_apm/support/transformer.rb

@@ -40,10 +40,6 @@ def self.trace_flags_from_boolean(trace_flags)
         trace_flags == true ? "01" : "00"
       end
 
-      def self.sampled?(decision)
-        decision == ::OpenTelemetry::SDK::Trace::Samplers::Decision::RECORD_AND_SAMPLE
-      end
-
       def self.span_id_from_sw(sw_value)
         sw_value.split("-")[0]
       end

test/api/set_transaction_name_test.rb

@@ -0,0 +1,45 @@
+# Copyright (c) 2023 SolarWinds, LLC.
+# All rights reserved.
+
+require 'minitest_helper'
+require './lib/solarwinds_apm/support/txn_name_manager'
+require './lib/solarwinds_apm/opentelemetry'
+require './lib/solarwinds_apm/otel_config'
+require './lib/solarwinds_apm/constants'
+require './lib/solarwinds_apm/api'
+
+describe 'SolarWinds Set Transaction Name Test' do
+  before do
+    @span = create_span
+    SolarWindsAPM::OTelConfig.initialize
+    @processors = ::OpenTelemetry.tracer_provider.instance_variable_get(:@span_processors)
+    @solarwinds_processor = @processors.last
+    @solarwinds_processor.txn_manager.del("77cb6ccc522d3106114dd6ecbb70036a-31e175128efc4018")
+  end
+
+  after do
+    @span.context.trace_flags.instance_variable_set(:@flags, 0)
+  end
+
+  it 'calculate_transaction_names_with_unsampled_span' do
+    @solarwinds_processor.on_start(@span, ::OpenTelemetry::Context.current)
+    result = SolarWindsAPM::API.set_transaction_name('abcdf')
+    _(result).must_equal false
+    _(@solarwinds_processor.txn_manager.get("77cb6ccc522d3106114dd6ecbb70036a-31e175128efc4018")).must_equal "abcdf"
+  end
+
+  it 'calculate_transaction_names_with_empty_transaction_name' do
+    @solarwinds_processor.on_start(@span, ::OpenTelemetry::Context.current)
+    result = SolarWindsAPM::API.set_transaction_name('')
+    _(result).must_equal false
+    assert_nil(@solarwinds_processor.txn_manager.get("77cb6ccc522d3106114dd6ecbb70036a-31e175128efc4018"))
+  end
+
+  it 'calculate_transaction_names_with_sampled_span' do
+    @span.context.trace_flags.instance_variable_set(:@flags, 1)
+    @solarwinds_processor.on_start(@span, ::OpenTelemetry::Context.current)
+    result = SolarWindsAPM::API.set_transaction_name('abcdf')
+    _(result).must_equal true
+    _(@solarwinds_processor.txn_manager.get("77cb6ccc522d3106114dd6ecbb70036a-31e175128efc4018")).must_equal "abcdf"
+  end
+end

test/component/solarwinds_processor_test.rb

@@ -63,15 +63,4 @@
     _(::OpenTelemetry::Baggage.value(::SolarWindsAPM::Constants::INTL_SWO_CURRENT_TRACE_ID)).must_equal '77cb6ccc522d3106114dd6ecbb70036a'
     _(::OpenTelemetry::Baggage.value(::SolarWindsAPM::Constants::INTL_SWO_CURRENT_SPAN_ID)).must_equal '31e175128efc4018'
   end
-
-  # it 'calculate_transaction_names_with_custom_naming' do
-  #   clean_old_setting
-  #   SolarWindsAPM::OTelConfig.initialize
-  #   processors = ::OpenTelemetry.tracer_provider.instance_variable_get(:@span_processors)
-  #   solarwinds_processor = processors.last
-  #   solarwinds_processor.on_start(@span, ::OpenTelemetry::Context.current)
-  #   SolarWindsAPM::API.set_transaction_name('abcdf')
-  #   _(solarwinds_processor.txn_manager.get("77cb6ccc522d3106114dd6ecbb70036a-31e175128efc4018")).must_equal "abcdf"
-  # end
-
 end

test/component/solarwinds_sampler_test.rb

@@ -32,9 +32,13 @@
 
   end
 
-  it 'test_calculate_attributes should return nil' do 
-    attributes = @sampler.send(:calculate_attributes, "tmp_span", @attributes_dict, @decision, @tracestate, @parent_context)
-    assert_nil(attributes)
+  it 'test_calculate_attributes' do 
+    attributes = @sampler.send(:calculate_attributes, @attributes_dict, @decision, @tracestate, @parent_context, @xtraceoptions)
+    _(attributes['a']).must_equal 'b'
+    _(attributes['BucketCapacity']).must_equal ''
+    _(attributes['BucketRate']).must_equal ''
+    _(attributes['SampleRate']).must_equal nil
+    _(attributes['SampleSource']).must_equal nil
   end
 
   it 'test_add_tracestate_capture_to_attributes_dict with sw.w3c.tracestate' do 
@@ -186,4 +190,15 @@
 
   end
 
+  it 'test_should_sample?' do
+    should_sample = @sampler.send(:otel_sampled?, ::OpenTelemetry::SDK::Trace::Samplers::Decision::RECORD_AND_SAMPLE)
+    assert(should_sample)
+
+    should_sample = @sampler.send(:otel_sampled?, ::OpenTelemetry::SDK::Trace::Samplers::Decision::DROP)
+    _(should_sample).must_equal false
+
+    should_sample = @sampler.send(:otel_sampled?, ::OpenTelemetry::SDK::Trace::Samplers::Decision::RECORD_ONLY)
+    _(should_sample).must_equal false
+  end
+
 end

test/unit/transformer_test.rb

@@ -40,14 +40,6 @@
     _(result).must_equal "00"
   end
 
-  it 'test sampled?' do 
-    result = @transformer.sampled?(::OpenTelemetry::SDK::Trace::Samplers::Decision::RECORD_AND_SAMPLE)
-    _(result).must_equal true
-
-    result = @transformer.sampled?(::OpenTelemetry::SDK::Trace::Samplers::Decision::RECORD_ONLY)
-    _(result).must_equal false
-  end
-
   it 'test span_id_from_sw' do 
     result = @transformer.span_id_from_sw("a-b")
     _(result).must_equal "a"