wip - update span processor

Author: solnic

lib/sentry/opentelemetry/span_processor.ex

@@ -27,7 +27,13 @@ if Sentry.OpenTelemetry.VersionChecker.tracing_compatible?() do
 
       SpanStorage.store_span(span_record)
 
-      if span_record.parent_span_id == nil do
+      # Check if this is a root span (no parent) or a transaction root (HTTP server request span)
+      # HTTP server request spans should be treated as transaction roots even when they have
+      # an external parent span ID (from distributed tracing)
+      is_transaction_root =
+        span_record.parent_span_id == nil or is_http_server_request_span?(span_record)
+
+      if is_transaction_root do
         child_span_records = SpanStorage.get_child_spans(span_record.span_id)
         transaction = build_transaction(span_record, child_span_records)
 
@@ -47,8 +53,18 @@ if Sentry.OpenTelemetry.VersionChecker.tracing_compatible?() do
               {:error, :invalid_span}
           end
 
+        # Clean up: remove the transaction root span and all its children
+        # Note: For distributed tracing, the transaction root span may have been stored
+        # as a child span (with a remote parent_span_id). In that case, we need to also
+        # remove it from the child spans, not just look for it as a root span.
         :ok = SpanStorage.remove_root_span(span_record.span_id)
 
+        if span_record.parent_span_id != nil do
+          # This span was stored as a child because it has a remote parent (distributed tracing).
+          # We need to explicitly remove it from the child spans storage.
+          :ok = SpanStorage.remove_child_span(span_record.parent_span_id, span_record.span_id)
+        end
+
         result
       else
         true
@@ -60,6 +76,13 @@ if Sentry.OpenTelemetry.VersionChecker.tracing_compatible?() do
       :ok
     end
 
+    # Helper function to detect if a span represents an HTTP server request
+    # that should be treated as a transaction root for distributed tracing
+    defp is_http_server_request_span?(%{kind: kind, attributes: attributes}) do
+      kind == :server and
+        Map.has_key?(attributes, to_string(HTTPAttributes.http_request_method()))
+    end
+
     defp build_transaction(root_span_record, child_span_records) do
       root_span = build_span(root_span_record)
       child_spans = Enum.map(child_span_records, &build_span(&1))

lib/sentry/opentelemetry/span_storage.ex

@@ -121,6 +121,16 @@ if Sentry.OpenTelemetry.VersionChecker.tracing_compatible?() do
       :ok
     end
 
+    @spec remove_child_span(String.t(), String.t(), keyword()) :: :ok
+    def remove_child_span(parent_span_id, span_id, opts \\ []) do
+      table_name = Keyword.get(opts, :table_name, default_table_name())
+      key = {:child_span, parent_span_id, span_id}
+
+      :ets.delete(table_name, key)
+
+      :ok
+    end
+
     defp schedule_cleanup(interval) do
       Process.send_after(self(), :cleanup_stale_spans, interval)
     end

test/sentry/opentelemetry/span_processor_test.exs

@@ -310,5 +310,153 @@ defmodule Sentry.Opentelemetry.SpanProcessorTest do
 
       Application.put_env(:opentelemetry, :sampler, original_sampler)
     end
+
+    @tag span_storage: true
+    test "treats HTTP server request spans as transaction roots for distributed tracing" do
+      put_test_config(environment_name: "test", traces_sample_rate: 1.0)
+
+      Sentry.Test.start_collecting_sentry_reports()
+
+      require OpenTelemetry.Tracer, as: Tracer
+      require OpenTelemetry.SemConv.Incubating.HTTPAttributes, as: HTTPAttributes
+      require OpenTelemetry.SemConv.Incubating.URLAttributes, as: URLAttributes
+
+      # Simulate an incoming HTTP request with an external parent span ID (from browser/client)
+      # This represents a distributed trace where the client started the trace
+      external_trace_id = 0x1234567890ABCDEF1234567890ABCDEF
+      external_parent_span_id = 0xABCDEF1234567890
+
+      # Create a remote parent span context using :otel_tracer.from_remote_span
+      remote_parent = :otel_tracer.from_remote_span(external_trace_id, external_parent_span_id, 1)
+
+      ctx = Tracer.set_current_span(:otel_ctx.new(), remote_parent)
+
+      # Start an HTTP server span with the remote parent context
+      Tracer.with_span ctx, "POST /api/users", %{
+        kind: :server,
+        attributes: %{
+          HTTPAttributes.http_request_method() => :POST,
+          URLAttributes.url_path() => "/api/users",
+          "http.route" => "/api/users",
+          "server.address" => "localhost",
+          "server.port" => 4000
+        }
+      } do
+        # Simulate child spans (database queries, etc.)
+        Tracer.with_span "db.query:users", %{
+          kind: :client,
+          attributes: %{
+            "db.system" => :postgresql,
+            "db.statement" => "INSERT INTO users (name) VALUES ($1)"
+          }
+        } do
+          Process.sleep(10)
+        end
+
+        Tracer.with_span "db.query:notifications", %{
+          kind: :client,
+          attributes: %{
+            "db.system" => :postgresql,
+            "db.statement" => "INSERT INTO notifications (user_id) VALUES ($1)"
+          }
+        } do
+          Process.sleep(10)
+        end
+      end
+
+      # Should capture the HTTP request span as a transaction root despite having an external parent
+      assert [%Sentry.Transaction{} = transaction] = Sentry.Test.pop_sentry_transactions()
+
+      # Verify transaction properties
+      assert transaction.transaction == "POST /api/users"
+      assert transaction.transaction_info == %{source: :custom}
+      assert length(transaction.spans) == 2
+
+      # Verify child spans are properly included
+      span_ops = Enum.map(transaction.spans, & &1.op) |> Enum.sort()
+      assert span_ops == ["db", "db"]
+
+      # Verify child spans have detailed data (like SQL queries)
+      [span1, span2] = transaction.spans
+      assert span1.description =~ "INSERT INTO"
+      assert span2.description =~ "INSERT INTO"
+      assert span1.data["db.system"] == :postgresql
+      assert span2.data["db.system"] == :postgresql
+      assert span1.data["db.statement"] =~ "INSERT INTO users"
+      assert span2.data["db.statement"] =~ "INSERT INTO notifications"
+
+      # Verify all spans share the same trace ID (from the external parent)
+      trace_id = transaction.contexts.trace.trace_id
+
+      Enum.each(transaction.spans, fn span ->
+        assert span.trace_id == trace_id
+      end)
+
+      # The transaction should have the external parent's trace ID
+      assert transaction.contexts.trace.trace_id ==
+               "1234567890abcdef1234567890abcdef"
+    end
+
+    @tag span_storage: true
+    test "cleans up HTTP server span and children after sending distributed trace transaction", %{
+      table_name: table_name
+    } do
+      put_test_config(environment_name: "test", traces_sample_rate: 1.0)
+
+      Sentry.Test.start_collecting_sentry_reports()
+
+      require OpenTelemetry.Tracer, as: Tracer
+      require OpenTelemetry.SemConv.Incubating.HTTPAttributes, as: HTTPAttributes
+      require OpenTelemetry.SemConv.Incubating.URLAttributes, as: URLAttributes
+
+      # Simulate an incoming HTTP request with an external parent span ID (from browser/client)
+      external_trace_id = 0x1234567890ABCDEF1234567890ABCDEF
+      external_parent_span_id = 0xABCDEF1234567890
+
+      remote_parent = :otel_tracer.from_remote_span(external_trace_id, external_parent_span_id, 1)
+      ctx = Tracer.set_current_span(:otel_ctx.new(), remote_parent)
+
+      # Start an HTTP server span with the remote parent context
+      Tracer.with_span ctx, "POST /api/users", %{
+        kind: :server,
+        attributes: %{
+          HTTPAttributes.http_request_method() => :POST,
+          URLAttributes.url_path() => "/api/users"
+        }
+      } do
+        # Simulate child spans (database queries, etc.)
+        Tracer.with_span "db.query:users", %{
+          kind: :client,
+          attributes: %{
+            "db.system" => :postgresql,
+            "db.statement" => "INSERT INTO users (name) VALUES ($1)"
+          }
+        } do
+          Process.sleep(10)
+        end
+      end
+
+      # Should capture the HTTP request span as a transaction
+      assert [%Sentry.Transaction{} = transaction] = Sentry.Test.pop_sentry_transactions()
+
+      # Verify the HTTP server span was removed from storage
+      # (even though it was stored as a child span due to having a remote parent)
+      http_server_span_id = transaction.contexts.trace.span_id
+      remote_parent_span_id_str = "abcdef1234567890"
+
+      # The HTTP server span should not exist in storage anymore
+      assert SpanStorage.get_root_span(http_server_span_id, table_name: table_name) == nil
+
+      # Check that it was also removed from child spans storage
+      # We can't directly check if a specific child was removed, but we can verify
+      # that get_child_spans for the remote parent returns empty (or doesn't include our span)
+      remaining_children =
+        SpanStorage.get_child_spans(remote_parent_span_id_str, table_name: table_name)
+
+      refute Enum.any?(remaining_children, fn span -> span.span_id == http_server_span_id end)
+
+      # Verify child spans of the HTTP server span were also removed
+      assert [] == SpanStorage.get_child_spans(http_server_span_id, table_name: table_name)
+    end
   end
 end