From 5cae8f1fa49bd614d3f044b8136e4c21b75aa800 Mon Sep 17 00:00:00 2001
From: Francis Bogsanyi <francis.bogsanyi@shopify.com>
Date: Mon, 25 Aug 2025 15:09:22 -0400
Subject: [PATCH] This PR corrects the spec-compliance of API
 Tracer.start_span.

The spec states:
> The API MUST return a non-recording Span with the SpanContext in the
  parent Context (whether explicitly given or implicit current). If the
  Span in the parent Context is already non-recording, it SHOULD be
  returned directly without instantiating a new Span. If the parent
  Context contains no Span, an empty non-recording Span MUST be returned
  instead (i.e., having a SpanContext with all-zero Span and Trace IDs,
  empty Tracestate, and unsampled TraceFlags).

With new tests that match the spec, the current implementation fails
with:

``` OpenTelemetry::Trace::Tracer::#start_span#test_0002_returns a
non-recording span with the parent span context if the parent span is
recording [test/opentelemetry/trace/tracer_test.rb:97]: Expected
@context=#<OpenTelemetry::Trace::SpanContext:0x0000000120e17140
@trace_id="E\xC6d\xD5S+h\x8B\xC6u\xE6\xD5\ePQ\xDD",
@span_id="\xE2\xB3\x01\x99\xEC\xA0\xC1\x81",
@trace_flags=#<OpenTelemetry::Trace::TraceFlags:0x00000001052a50c0
@flags=0>,
@tracestate=#<OpenTelemetry::Trace::Tracestate:0x000000012085fdd8
@hash={}>, @remote=false>> to not be equal to
@context=#<OpenTelemetry::Trace::SpanContext:0x0000000120e17140
@trace_id="E\xC6d\xD5S+h\x8B\xC6u\xE6\xD5\ePQ\xDD",
@span_id="\xE2\xB3\x01\x99\xEC\xA0\xC1\x81",
@trace_flags=#<OpenTelemetry::Trace::TraceFlags:0x00000001052a50c0
@flags=0>,
@tracestate=#<OpenTelemetry::Trace::Tracestate:0x000000012085fdd8
@hash={}>, @remote=false>>. ```

The new implementation correctly passes this test (and the old tests).
Shopify has been running with a similar patch in production for several
years, based on a belief that the spec was incorrect - instead the
_implementation_ was wrong.
---
 api/lib/opentelemetry/trace/tracer.rb       |  9 ++++--
 api/test/opentelemetry/trace/tracer_test.rb | 36 +++++++++++++++------
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/api/lib/opentelemetry/trace/tracer.rb b/api/lib/opentelemetry/trace/tracer.rb
index 2a5f311946..2e30e87c9b 100644
--- a/api/lib/opentelemetry/trace/tracer.rb
+++ b/api/lib/opentelemetry/trace/tracer.rb
@@ -58,10 +58,13 @@ 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)
         span = OpenTelemetry::Trace.current_span(with_parent)
 
-        if span.context.valid?
-          span
+        if span.recording?
+          OpenTelemetry::Trace.non_recording_span(span.context)
         else
-          Span::INVALID
+          # Either the span is valid and non-recording, in which case we return it,
+          # or there was no span in the Context and Trace.current_span returned Span::INVALID,
+          # which is what we're supposed to return.
+          span
         end
       end
     end
diff --git a/api/test/opentelemetry/trace/tracer_test.rb b/api/test/opentelemetry/trace/tracer_test.rb
index 54228e4778..b242bf2e9c 100644
--- a/api/test/opentelemetry/trace/tracer_test.rb
+++ b/api/test/opentelemetry/trace/tracer_test.rb
@@ -72,22 +72,38 @@ def start_span(*)
   end
 
   describe '#start_span' do
-    it 'returns a valid span with the parent context' do
-      span = tracer.start_span('op', with_parent: parent_context)
-      _(span.context).must_be :valid?
-      _(span.context).must_equal(parent_span_context)
+    # Specification:
+    # The API MUST return a non-recording Span with the SpanContext in the parent Context
+    #  (whether explicitly given or implicit current). If the Span in the parent Context
+    #  is already non-recording, it SHOULD be returned directly without instantiating a
+    #  new Span. If the parent Context contains no Span, an empty non-recording Span MUST
+    #  be returned instead (i.e., having a SpanContext with all-zero Span and Trace IDs,
+    #  empty Tracestate, and unsampled TraceFlags).
+
+    it 'should return the parent span directly if already not recording' do
+      parent_span = OpenTelemetry::Trace::Span.new
+      with_parent = OpenTelemetry::Trace.context_with_span(parent_span, parent_context: OpenTelemetry::Context.empty)
+      span = tracer.start_span('op', with_parent: with_parent)
+      _(span).must_equal(parent_span)
+      _(span).wont_be :recording?
     end
 
-    it 'returns an invalid unsampled span by default' do
-      span = tracer.start_span('op')
-      _(span.context).wont_be :valid?
-      _(span.context.trace_flags).wont_be :sampled?
+    it 'returns a non-recording span with the parent span context if the parent span is recording' do
+      parent_span = OpenTelemetry::Trace::Span.new
+      span = parent_span.stub(:recording?, true) do
+        with_parent = OpenTelemetry::Trace.context_with_span(parent_span, parent_context: OpenTelemetry::Context.empty)
+        tracer.start_span('op', with_parent: with_parent)
+      end
+      _(span).wont_equal(parent_span)
+      _(span.context).must_equal(parent_span.context)
+      _(span).wont_be :recording?
     end
 
-    it 'returns an invalid unsampled span when passed an invalid parent context' do
-      span = tracer.start_span('op', with_parent: invalid_parent_context)
+    it 'returns an invalid unsampled span when no parent is provided' do
+      span = tracer.start_span('op', with_parent: OpenTelemetry::Context.empty)
       _(span.context).wont_be :valid?
       _(span.context.trace_flags).wont_be :sampled?
+      _(span).wont_be :recording?
     end
   end
 end