NH-52516: return false if span is not sampled or invalid #59
Conversation
|
I don't think this is right. We should still allow a custom transaction name if not sampled (i.e. record only), so that it populates metrics. |
…actor a little bit of sampler include its test
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -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 | |||
There was a problem hiding this comment.
related to sampler refactor. instead of checking is otel-type sampled in calculate_attributes; just check in function should_sample?
| @@ -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) | |||
There was a problem hiding this comment.
related to sampler refactor.
| 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 |
There was a problem hiding this comment.
related to sampler refactor.
Thanks, I rewrote the part that will still set transaction name while it's not sampled, but the function will return false. |
| 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) |
There was a problem hiding this comment.
A future improvement could be using a single Baggage key to hold the trace info
| 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 |
There was a problem hiding this comment.
If you used a single baggage key, this check could also be simplified
99-00000000000000000000000000000000-0000000000000000-00to separate from start span00-00000000000000000000000000000000-0000000000000000-00