From fb7b73105453d82f6f5080c0481c7e3d1681cd59 Mon Sep 17 00:00:00 2001 From: Ed Bond Date: Sat, 12 Jul 2025 16:23:14 +0000 Subject: [PATCH 1/2] distributed tracing support with the parent id set before --- lib/sentry/opentelemetry/span_processor.ex | 26 ++++++-- .../opentelemetry/span_processor_test.exs | 66 +++++++++++++++++++ 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/lib/sentry/opentelemetry/span_processor.ex b/lib/sentry/opentelemetry/span_processor.ex index af16ac02..4d72ba1b 100644 --- a/lib/sentry/opentelemetry/span_processor.ex +++ b/lib/sentry/opentelemetry/span_processor.ex @@ -15,19 +15,22 @@ if Code.ensure_loaded?(OpenTelemetry) do alias Sentry.{Transaction, OpenTelemetry.SpanStorage, OpenTelemetry.SpanRecord} alias Sentry.Interfaces.Span - # This can be a no-op since we can postpone inserting the span into storage until on_end + # This can be a no-op since we can postpone inserting the span into storage until on_end @impl :otel_span_processor def on_start(_ctx, otel_span, _config) do otel_span end - @impl :otel_span_processor + @impl :otel_span_processor def on_end(otel_span, _config) do span_record = SpanRecord.new(otel_span) 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 request span) + is_transaction_root = span_record.parent_span_id == nil or is_http_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) @@ -60,7 +63,20 @@ if Code.ensure_loaded?(OpenTelemetry) do :ok end - defp build_transaction(root_span_record, child_span_records) do + # Helper function to detect if a span represents an HTTP request that should be treated as a transaction root + defp is_http_request_span?(%{attributes: attributes, name: name}) do + # Check if this is an HTTP request span (has HTTP method and is likely a server span) + has_http_method = Map.has_key?(attributes, to_string(HTTPAttributes.http_request_method())) + has_http_route = Map.has_key?(attributes, "http.route") + has_url_path = Map.has_key?(attributes, to_string(URLAttributes.url_path())) + + # Check if the name looks like an HTTP endpoint + name_looks_like_http = String.contains?(name, ["/", "POST", "GET", "PUT", "DELETE", "PATCH"]) + + (has_http_method and (has_http_route or has_url_path)) or name_looks_like_http + 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)) @@ -77,7 +93,7 @@ if Code.ensure_loaded?(OpenTelemetry) do }) end - defp transaction_name( + defp transaction_name( %{attributes: %{unquote(to_string(MessagingAttributes.messaging_system())) => :oban}} = span_record ) do diff --git a/test/sentry/opentelemetry/span_processor_test.exs b/test/sentry/opentelemetry/span_processor_test.exs index 331acd08..3e767700 100644 --- a/test/sentry/opentelemetry/span_processor_test.exs +++ b/test/sentry/opentelemetry/span_processor_test.exs @@ -234,6 +234,72 @@ defmodule Sentry.Opentelemetry.SpanProcessorTest do assert length(transaction.spans) == 0 assert transaction.transaction == "child_instrumented_function_standalone" end + + @tag span_storage: true + test "treats HTTP request spans as transaction roots even with external parents" do + put_test_config(environment_name: "test", traces_sample_rate: 1.0) + + Sentry.Test.start_collecting_sentry_reports() + + # Simulate an HTTP request span with external parent (like from browser tracing) + require OpenTelemetry.Tracer, as: Tracer + require OpenTelemetry.SemConv.Incubating.HTTPAttributes, as: HTTPAttributes + require OpenTelemetry.SemConv.Incubating.URLAttributes, as: URLAttributes + + # Create a span with HTTP attributes and an external parent span ID + external_parent_span_id = "b943d7459127970c" + + # Start a span that simulates an HTTP request from an external trace + Tracer.with_span "POST /api/v1alpha", %{ + attributes: %{ + HTTPAttributes.http_request_method() => :POST, + URLAttributes.url_path() => "/api/v1alpha", + "http.route" => "/api/v1alpha", + "server.address" => "localhost", + "server.port" => 4000 + }, + parent: {:span_context, :undefined, external_parent_span_id, :undefined, :undefined, :undefined, :undefined, :undefined, :undefined, :undefined} + } do + # Simulate child spans (database queries, etc.) with proper DB attributes + Tracer.with_span "matrix_data.repo.query", %{ + attributes: %{ + "db.system" => :postgresql, + "db.statement" => "SELECT * FROM users" + } + } do + Process.sleep(10) + end + + Tracer.with_span "matrix_data.repo.query:agents", %{ + attributes: %{ + "db.system" => :postgresql, + "db.statement" => "INSERT INTO agents (...) VALUES (...)" + } + } 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/v1alpha" + assert transaction.transaction_info == %{source: :custom} + assert length(transaction.spans) == 2 + + # Verify child spans are properly included - they should have "db" operation + span_names = Enum.map(transaction.spans, & &1.op) |> Enum.sort() + expected_names = ["db", "db"] + assert span_names == expected_names + + # Verify all spans share the same trace ID + trace_id = transaction.contexts.trace.trace_id + Enum.each(transaction.spans, fn span -> + assert span.trace_id == trace_id + end) + end + @tag span_storage: true test "concurrent traces maintain independent sampling decisions" do From c781dcbf63d37c695dc99b27a2e6fe42ad3bbc6a Mon Sep 17 00:00:00 2001 From: Ed Bond Date: Sat, 12 Jul 2025 16:32:22 +0000 Subject: [PATCH 2/2] formatting --- lib/sentry/opentelemetry/span_processor.ex | 14 ++++++++------ test/sentry/opentelemetry/span_processor_test.exs | 12 +++++++----- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/sentry/opentelemetry/span_processor.ex b/lib/sentry/opentelemetry/span_processor.ex index 4d72ba1b..ea42d86c 100644 --- a/lib/sentry/opentelemetry/span_processor.ex +++ b/lib/sentry/opentelemetry/span_processor.ex @@ -15,20 +15,21 @@ if Code.ensure_loaded?(OpenTelemetry) do alias Sentry.{Transaction, OpenTelemetry.SpanStorage, OpenTelemetry.SpanRecord} alias Sentry.Interfaces.Span - # This can be a no-op since we can postpone inserting the span into storage until on_end + # This can be a no-op since we can postpone inserting the span into storage until on_end @impl :otel_span_processor def on_start(_ctx, otel_span, _config) do otel_span end - @impl :otel_span_processor + @impl :otel_span_processor def on_end(otel_span, _config) do span_record = SpanRecord.new(otel_span) SpanStorage.store_span(span_record) # Check if this is a root span (no parent) or a transaction root (HTTP request span) - is_transaction_root = span_record.parent_span_id == nil or is_http_request_span?(span_record) + is_transaction_root = + span_record.parent_span_id == nil or is_http_request_span?(span_record) if is_transaction_root do child_span_records = SpanStorage.get_child_spans(span_record.span_id) @@ -71,12 +72,13 @@ if Code.ensure_loaded?(OpenTelemetry) do has_url_path = Map.has_key?(attributes, to_string(URLAttributes.url_path())) # Check if the name looks like an HTTP endpoint - name_looks_like_http = String.contains?(name, ["/", "POST", "GET", "PUT", "DELETE", "PATCH"]) + name_looks_like_http = + String.contains?(name, ["/", "POST", "GET", "PUT", "DELETE", "PATCH"]) (has_http_method and (has_http_route or has_url_path)) or name_looks_like_http end - defp build_transaction(root_span_record, child_span_records) do + 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)) @@ -93,7 +95,7 @@ if Code.ensure_loaded?(OpenTelemetry) do }) end - defp transaction_name( + defp transaction_name( %{attributes: %{unquote(to_string(MessagingAttributes.messaging_system())) => :oban}} = span_record ) do diff --git a/test/sentry/opentelemetry/span_processor_test.exs b/test/sentry/opentelemetry/span_processor_test.exs index 3e767700..ef8c3250 100644 --- a/test/sentry/opentelemetry/span_processor_test.exs +++ b/test/sentry/opentelemetry/span_processor_test.exs @@ -234,7 +234,7 @@ defmodule Sentry.Opentelemetry.SpanProcessorTest do assert length(transaction.spans) == 0 assert transaction.transaction == "child_instrumented_function_standalone" end - + @tag span_storage: true test "treats HTTP request spans as transaction roots even with external parents" do put_test_config(environment_name: "test", traces_sample_rate: 1.0) @@ -248,7 +248,7 @@ defmodule Sentry.Opentelemetry.SpanProcessorTest do # Create a span with HTTP attributes and an external parent span ID external_parent_span_id = "b943d7459127970c" - + # Start a span that simulates an HTTP request from an external trace Tracer.with_span "POST /api/v1alpha", %{ attributes: %{ @@ -258,7 +258,9 @@ defmodule Sentry.Opentelemetry.SpanProcessorTest do "server.address" => "localhost", "server.port" => 4000 }, - parent: {:span_context, :undefined, external_parent_span_id, :undefined, :undefined, :undefined, :undefined, :undefined, :undefined, :undefined} + parent: + {:span_context, :undefined, external_parent_span_id, :undefined, :undefined, :undefined, + :undefined, :undefined, :undefined, :undefined} } do # Simulate child spans (database queries, etc.) with proper DB attributes Tracer.with_span "matrix_data.repo.query", %{ @@ -269,7 +271,7 @@ defmodule Sentry.Opentelemetry.SpanProcessorTest do } do Process.sleep(10) end - + Tracer.with_span "matrix_data.repo.query:agents", %{ attributes: %{ "db.system" => :postgresql, @@ -295,11 +297,11 @@ defmodule Sentry.Opentelemetry.SpanProcessorTest do # Verify all spans share the same trace ID trace_id = transaction.contexts.trace.trace_id + Enum.each(transaction.spans, fn span -> assert span.trace_id == trace_id end) end - @tag span_storage: true test "concurrent traces maintain independent sampling decisions" do