Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Update untraced to suppress logging "Calling finish on an ended Span" warnings #1620

Merged
merged 7 commits into from
Apr 23, 2024
Merged

fix: Update untraced to suppress logging "Calling finish on an ended Span" warnings #1620

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e9428ca
fix: Update `untraced` to suppress logging "Calling finish on an ende…
yoheyk Mar 21, 2024
a59dd0a
Merge remote-tracking branch 'origin/main' into fix-untraced-calling-…
yoheyk Apr 9, 2024
108bde4
add tests for untraced
yoheyk Apr 9, 2024
23e2e86
disable Metrics/PerceivedComplexity in TracerProvider#internal_start_…
yoheyk Apr 9, 2024
96ac5f6
remove setting ALWAYS_ON from test cases
yoheyk Apr 10, 2024
5f9b4a2
return a non-recording span without calling @sampler.should_sample
yoheyk Apr 18, 2024
216e795
Merge branch 'main' into fix-untraced-calling-on-an-ended-span
fbogsany Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions sdk/lib/opentelemetry/sdk/trace/tracer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ def start_root_span(name, attributes: nil, links: nil, start_timestamp: nil, kin

def start_span(name, with_parent: nil, attributes: nil, links: nil, start_timestamp: nil, kind: nil)
with_parent ||= Context.current
return super(name, with_parent: with_parent, attributes: attributes, links: links, start_timestamp: start_timestamp, kind: kind) if Common::Utilities.untraced?(with_parent)

name ||= 'empty'
kind ||= :internal

Expand Down
4 changes: 2 additions & 2 deletions sdk/lib/opentelemetry/sdk/trace/tracer_provider.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ def add_span_processor(span_processor)
end

# @api private
def internal_start_span(name, kind, attributes, links, start_timestamp, parent_context, instrumentation_scope) # rubocop:disable Metrics/MethodLength
def internal_start_span(name, kind, attributes, links, start_timestamp, parent_context, instrumentation_scope) # rubocop:disable Metrics/MethodLength, Metrics/PerceivedComplexity
parent_span = OpenTelemetry::Trace.current_span(parent_context)
parent_span_context = parent_span.context

Expand All @@ -138,7 +138,7 @@ def internal_start_span(name, kind, attributes, links, start_timestamp, parent_c
trace_id ||= @id_generator.generate_trace_id
result = @sampler.should_sample?(trace_id: trace_id, parent_context: parent_context, links: links, name: name, kind: kind, attributes: attributes)
span_id = @id_generator.generate_span_id
if result.recording? && !@stopped
if !OpenTelemetry::Common::Utilities.untraced?(parent_context) && result.recording? && !@stopped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should return a non-recording span without calling @sampler.should_sample if untraced?(parent_context).

Rationale:

  1. We should not create a non-recording span using the tracestate from a sampler whose result we're overriding.
  2. We should re-use the span ID from the parent span (if there is one) to avoid breaking the trace (in case we propagate the trace context with an outbound request and the server makes it own sampling decision).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this? 5f9b4a2

trace_flags = result.sampled? ? OpenTelemetry::Trace::TraceFlags::SAMPLED : OpenTelemetry::Trace::TraceFlags::DEFAULT
context = OpenTelemetry::Trace::SpanContext.new(trace_id: trace_id, span_id: span_id, trace_flags: trace_flags, tracestate: result.tracestate)
attributes = attributes&.merge(result.attributes) || result.attributes.dup
Expand Down
21 changes: 21 additions & 0 deletions sdk/test/opentelemetry/sdk/trace/tracer_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,5 +329,26 @@
_(span.status.code).must_equal(OpenTelemetry::Trace::Status::ERROR)
_(span.status.description).must_equal('Unhandled exception of type: RuntimeError')
end

it 'yields a no-op span within an untraced block' do
tracer.in_span('root') do
OpenTelemetry::Common::Utilities.untraced do
tracer.in_span('op') do |span|
_(span).must_be_instance_of(OpenTelemetry::Trace::Span)
_(span.context.trace_flags).wont_be :sampled?
_(span).wont_be :recording?
end
end
end
end

it 'does not log "Calling finish on an ended Span" warnings' do
OpenTelemetry::TestHelpers.with_test_logger do |log_stream|
tracer.in_span('root') do
OpenTelemetry::Common::Utilities.untraced { tracer.in_span('op') {} }
end
_(log_stream.string).wont_include 'Calling finish on an ended Span'
end
end
end
end