Add SpanProcessor for OpenTelemetry #875

solnic · 2025-03-14T10:15:23Z

To make this work you need to add opentelemetry deps:

      {:opentelemetry, "~> 1.3"},
      {:opentelemetry_api, "~> 1.2"},
      {:opentelemetry_exporter, "~> 1.6"},
      {:opentelemetry_phoenix, "~> 2.0"},
      {:opentelemetry_ecto, "~> 1.2"},
      {:opentelemetry_bandit, "~> 0.2"}

Then configure our span processor:

config :opentelemetry, span_processor: {Sentry.OpenTelemetry.SpanProcessor, []}

Things should start looking more or less like this:

solnic · 2025-03-14T14:09:34Z

1) test loads in_app_module_allow_list (Public.Sentry.ConfigTest)
Error:      apps/public/test/public_test.exs:4
     ** (RuntimeError) the Sentry configuration seems to be not available (while trying to fetch :in_app_module_allow_list). This is likely because the :sentry application has not been started yet. Make sure that you start the :sentry application before using any of its functions.

     code: assert Sentry.Config.in_app_module_allow_list() |> Enum.sort() ==
     stacktrace:
       (sentry 10.8.1) lib/sentry/config.ex:698: Sentry.Config.in_app_module_allow_list/0
       test/public_test.exs:5: (test)


Finished in 0.00 seconds (0.00s async, 0.00s sync)
1 test, 1 failure
==> admin
Running ExUnit with seed: 25[95](https://github.com/getsentry/sentry-elixir/actions/runs/13856201920/job/38773483717?pr=875#step:10:96)1, max_cases: 8



  1) test loads in_app_module_allow_list (Admin.Sentry.ConfigTest)
Error:      apps/admin/test/admin_test.exs:4
     ** (RuntimeError) the Sentry configuration seems to be not available (while trying to fetch :in_app_module_allow_list). This is likely because the :sentry application has not been started yet. Make sure that you start the :sentry application before using any of its functions.

     code: assert Sentry.Config.in_app_module_allow_list() |> Enum.sort() ==
     stacktrace:
       (sentry 10.8.1) lib/sentry/config.ex:6[98](https://github.com/getsentry/sentry-elixir/actions/runs/13856201920/job/38773483717?pr=875#step:10:99): Sentry.Config.in_app_module_allow_list/0
       test/admin_test.exs:5: (test)

@whatyouhide ☝🏻 I'm having trouble figuring out these failures. Could you tell me how I could debug what exactly causes the :sentry app startup failure? We start it manually in the umbrella test helpers and for whatever reason it started failing when opentelemetry is included.

whatyouhide · 2025-03-17T08:30:38Z

Adding just the OTel deps without any other changes leads to the same issue?

sl0thentr0py · 2025-03-17T11:47:36Z

please write some minimal description in the PR about

what functionality this adds
instructions to add this to a Phoenix app with opentelemetry instrumentation and test it out

solnic · 2025-03-18T09:55:26Z

Adding just the OTel deps without any other changes leads to the same issue?

@whatyouhide @sl0thentr0py I narrowed it down to opentelemetry dep. When it's included, the app doesn't start in umbrella integration test. We need to make otel deps optional anyway, so it's time to address this. How do we want to approach it though? Should I create sentry-opentelemetry package in this repo?

solnic · 2025-03-18T10:32:11Z

@whatyouhide for the time being I addressed it by using optional deps for otel packages via 6fdf121 but then one of the tests in event_test.exs started to fail so I fixed it via d04c5d3 even though I don't understand what's going on there 🙃

whatyouhide · 2025-03-18T11:46:33Z

@solnic is this ready for review? It's still a draft

solnic · 2025-03-18T14:31:25Z

@solnic is this ready for review? It's still a draft

Not yet. I got it working but phoenix + bandit spans are not processed in a way that would make sense for Sentry for some reason. I've been investigating how to fix. It seems like phoenix spans are not coming in as children of bandit spans so there's a disconnect here. I'll figure it out 🙂

sl0thentr0py · 2025-03-18T14:37:33Z

We need to make otel deps optional anyway, so it's time to address this.

If we're just shipping a SpanProcessor first, having them as peer dependencies is more than fine. We just document well how to get otel up and running in parallel to sentry and hook them up properly.

We will revisit packaging later once we have a proper working prototype.

solnic · 2025-03-19T13:00:31Z

@sl0thentr0py @whatyouhide this is now open for reviews. I got it deployed to production already and it's working well (see screenshots in the description).

sl0thentr0py · 2025-03-25T18:16:25Z

okay I'm taking a preliminary look now

one thing we definitely need is that instead of the boolean Config.tracing we need

a traces_sample_rate
also ideally, a traces_sampler callback

without these, it is very hard for people to control quota spend so this is a hard requirement

sl0thentr0py

some suggestions, looks very good otherwise!

lib/sentry/opentelemetry/span_processor.ex

lib/sentry/opentelemetry/span_storage.ex

whatyouhide

Looking like a good start but still a bunch of work left to do. Let me know if any of the comments are not clear!

lib/sentry/config.ex

mix.exs

lib/sentry/opentelemetry/sampler.ex

lib/sentry/opentelemetry/span_record.ex

lib/sentry/opentelemetry/span_storage.ex

test/sentry/opentelemetry/sampler_test.exs

test/sentry/opentelemetry/span_processor_test.exs

lib/sentry/opentelemetry/span_processor.ex

whatyouhide · 2025-04-03T09:35:11Z

@solnic I’m assuming you're still working on this so re-request my review if this gets ready again.

solnic · 2025-04-03T09:38:36Z

@solnic I’m assuming you're still working on this so re-request my review if this gets ready again.

@whatyouhide yes, wrapping it up today, still a couple of things to address 🙃

solnic · 2025-04-03T11:03:49Z

also ideally, a traces_sampler callback

without these, it is very hard for people to control quota spend so this is a hard requirement

@sl0thentr0py I'll add traces_sampler config in a separate PR

sergiotapia · 2025-05-01T16:50:45Z

Thank you @solnic very excited for this release. I need this badly! Appreciate all your hard work!

whatyouhide

Did another round of review.

whatyouhide · 2025-05-06T12:37:27Z

lib/sentry/config.ex

+  @spec tracing?() :: boolean()
+  def tracing?, do: fetch!(:traces_sample_rate) > 0.0


I don't think this is the way to go, as it makes the API a little harder to work with IMO. I would rather have two separate configuration options if possible, unless other Sentry SDKs do it this way. If they do, can you point me to examples?

If they don't, let's go with:

tracing: boolean() for enabling and disabling tracing.

traces_sample_rate: float() (0.0 → 1.0), which can default to something other than 0.0 because now :tracing would default to false.

This way users can easily toggle tracing on and off without having to worry about the rate itself.

@whatyouhide actually, that's how this works across the SDKs, so we need to stay consistent over here

whatyouhide · 2025-05-06T12:38:55Z

lib/sentry/config.ex

+      will be sampled. This value is also used to determine if tracing is enabled: if it's
+      greater than `0`, tracing is enabled.
+
+      This feature requires `opentelemetry` package.


Let's always phrase this as something like

Suggested change

This feature requires `opentelemetry` package.

Tracing requires OpenTelemetry packages to work. See [the

OpenTelemetry setup documentation](LINK-TODO) for guides on

how to set it up.

whatyouhide · 2025-05-06T12:39:59Z

lib/sentry/client.ex

@@ -117,7 +117,7 @@ defmodule Sentry.Client do

    result_type = Keyword.get_lazy(opts, :result, &Config.send_result/0)
    client = Keyword.get_lazy(opts, :client, &Config.client/0)
-    sample_rate = Keyword.get_lazy(opts, :sample_rate, &Config.sample_rate/0)
+    sample_rate = Keyword.get_lazy(opts, :sample_rate, &Config.traces_sample_rate/0)


We should also rename the option that can be passed to this function to be :traces_sample_rate and document that.

@whatyouhide I'm gonna do a follow up PR with improved sampler which is meant to use this config option. @sl0thentr0py told me we need to sample sooner than in send_transaction and that the sampler is the correct place so we'd be dropping spans sooner rather than aggregating them in our storage to potentially drop later.

whatyouhide · 2025-05-06T12:48:28Z

lib/sentry/opentelemetry/span_processor.ex

+    alias Sentry.Interfaces.Span
+
+    # This can be a no-op since we can postpone inserting the span into storage until on_end
+    @impl true


Super nit, but can we replace these with

Suggested change

@impl true

@impl :otel_span_processor

just for ease of readability?

whatyouhide · 2025-05-06T12:50:31Z

lib/sentry/opentelemetry/span_processor.ex

+      SpanStorage.store_span(span_record)
+
+      if span_record.parent_span_id == nil do
+        child_span_records = SpanStorage.get_child_spans(span_record.span_id)


Should this somehow pop the child spans off of the storage? I’m worried about the concurrency of this all. I don't know how on_end/2 is called by OTel.

@whatyouhide OTel guarantees that on_end is not called for the same span more than once because it removes the span from its storage before sending a span to processors's on_end, so even if something may caused another call to on_end for the same span, that span will not be found and processor callbacks won't be called again.

whatyouhide · 2025-05-06T14:49:20Z

test/sentry/opentelemetry/span_processor_test.exs

+
+    def instrumented_function do
+      Tracer.with_span "instrumented_function" do
+        :timer.sleep(100)


What's the purpose of these sleeps? They're going to slow down tests but I’m not sure that they bring value to the tests?

whatyouhide · 2025-05-06T14:49:55Z

test/sentry/opentelemetry/span_processor_test.exs

+    assert nil ==
+             SpanStorage.get_root_span(transaction.contexts.trace.span_id, table_name: table_name)


Nit: we generally (almost always) do this

Suggested change

assert nil ==

SpanStorage.get_root_span(transaction.contexts.trace.span_id, table_name: table_name)

assert SpanStorage.get_root_span(transaction.contexts.trace.span_id, table_name: table_name) == nil

Applies below too.

whatyouhide · 2025-05-06T14:51:08Z

test/sentry/opentelemetry/span_processor_test.exs

+
+  defp assert_valid_trace_id(trace_id) do
+    assert is_binary(trace_id), "Expected trace_id to be a string"
+    assert String.length(trace_id) == 32, "Expected trace_id to be 32 characters long #{trace_id}"


Suggested change

assert String.length(trace_id) == 32, "Expected trace_id to be 32 characters long #{trace_id}"

assert byte_size(trace_id) == 32, "Expected trace_id to be 32 characters long #{trace_id}"

Otherwise this could (unlikely, ofc) catch Unicode characters.

whatyouhide · 2025-05-06T14:51:42Z

test/sentry/opentelemetry/span_processor_test.exs

+    assert String.match?(trace_id, ~r/^[a-f0-9]{32}$/),
+           "Expected trace_id to be a lowercase hex string"


You could simplify this by asserting on

assert {:ok, _} = Base.decode32(trace_id, case: :lower, padding: false)

?

No this actually does not pass

whatyouhide · 2025-05-06T14:52:25Z

test_integrations/phoenix_app/mix.exs

-      {:sentry, path: "../.."}
+      {:sentry, path: "../.."},
+
+      {:opentelemetry, "~> 1.5"},


Let's do these changes in the Phoenix test app in a separate PR that also, possibly, adds some tests.

sl0thentr0py · 2025-05-13T12:58:12Z

hey everyone, I will take one final look at this now and we are going to prioritize shipping a first version. Other nitpicks can be handled later and we will iterate on it. Thanks for all the reviews and discussions but this has been open for a fairly long time so now we should wrap it up.

@solnic told me he's been running it in his production app for a while so that's also good enough as a signal for me to get this out.

sl0thentr0py · 2025-05-13T13:08:10Z

lib/sentry/client.ex

@@ -117,7 +117,7 @@ defmodule Sentry.Client do

    result_type = Keyword.get_lazy(opts, :result, &Config.send_result/0)
    client = Keyword.get_lazy(opts, :client, &Config.client/0)
-    sample_rate = Keyword.get_lazy(opts, :sample_rate, &Config.sample_rate/0)
+    sample_rate = Keyword.get_lazy(opts, :sample_rate, &Config.traces_sample_rate/0)


the errors sample_rate and transactions traces_sample_rate are two different options and should be kept separate and used for the relevant sampling decision, so please don't replace the original one.

This comment is not correct. It's not replacing the original one, it's switching from sample_rate to traces_sample_rate in this function, which reports transactions. We didn't have traces_sample_rate before, so we used sample_rate as a stopgap.

I'm saying we need both

sl0thentr0py · 2025-05-13T13:09:21Z

lib/sentry/opentelemetry/sampler.ex

the sampler still needs to use traces_sample_rate right?

whatyouhide · 2025-05-14T10:15:32Z

As discussed in Discord, I won't re-review this. Thanks for all the work @solnic!

solnic linked an issue Mar 14, 2025 that may be closed by this pull request

Add SpanProcessor for OTel #874

Open

solnic force-pushed the 874-add-spanprocessor-for-otel branch from 0ebd132 to dee9540 Compare March 14, 2025 11:17

This was referenced Mar 14, 2025

Support for Traces/Transactions via Opentelemetry #784

Closed

OpenTelemetry Support #853

Closed

Performance powered by OTel #538

Open

solnic mentioned this pull request Mar 18, 2025

OpenTelemetry integration #843

Closed

solnic force-pushed the 874-add-spanprocessor-for-otel branch 2 times, most recently from b607ac1 to bdcc5de Compare March 19, 2025 12:45

solnic marked this pull request as ready for review March 19, 2025 12:59

solnic requested review from whatyouhide, savhappy and sl0thentr0py March 19, 2025 13:00

sl0thentr0py requested changes Mar 25, 2025

View reviewed changes

lib/sentry/opentelemetry/span_processor.ex Show resolved Hide resolved

lib/sentry/opentelemetry/span_storage.ex Outdated Show resolved Hide resolved

lib/sentry/opentelemetry/span_storage.ex Outdated Show resolved Hide resolved

whatyouhide requested changes Mar 30, 2025

View reviewed changes

solnic force-pushed the 874-add-spanprocessor-for-otel branch from 9fcc55e to 65c039c Compare April 2, 2025 13:18

solnic force-pushed the 874-add-spanprocessor-for-otel branch from c9152c6 to 1654724 Compare April 3, 2025 10:44

solnic force-pushed the 874-add-spanprocessor-for-otel branch from 01585a8 to 5e8fc99 Compare April 3, 2025 11:07

solnic added 15 commits April 29, 2025 14:02

Simplify stale span cleanup

84c056a

Refactor SpanStorage to allow custom table names

d951d22

No need to guard when creating ets table

bdf742d

Make SpanStorage more robust

565021f

Minor formatting tweak

bb84abc

tracing => traces_sample_rate

fb16f6b

Update docs for :traces_sample_rate option

398ce7a

Update doc

9912948

Add specs for pub functions in SpanStorage

4a2adbd

SpanProcessor test can't be async due to config change

1b01493

Make otel deps mandatory for phx test app

233eb52

Simplify span processor

3023f4b

Add missing type for span record

553083e

Fix spec for store_span

4b04434

Use System.unique_integer([:positive])

59e55a8

solnic force-pushed the 874-add-spanprocessor-for-otel branch from 0559b9e to 59e55a8 Compare April 29, 2025 14:04

whatyouhide reviewed May 6, 2025

View reviewed changes

Specify impl :otel_span_processor explicitly

e61e5c8

sl0thentr0py requested changes May 13, 2025

View reviewed changes

solnic added 5 commits May 13, 2025 13:36

Use warning not error when sending span fails

23fbc1e

Replace :timer.sleep with Process.sleep in span_processor_test

e08344f

Refactor assertions in span storage tests for clarity

9d6b9a2

Add more info to docs

6f56bc0

Use byte_size instead of String.length for trace_id validation

48fbd08

solnic requested review from sl0thentr0py and whatyouhide May 14, 2025 10:07

whatyouhide removed their request for review May 14, 2025 10:15

		@spec tracing?() :: boolean()
		def tracing?, do: fetch!(:traces_sample_rate) > 0.0

-      This feature requires `opentelemetry` package.
+      Tracing requires OpenTelemetry packages to work. See [the
+      OpenTelemetry setup documentation](LINK-TODO) for guides on
+      how to set it up.

		assert nil ==
		SpanStorage.get_root_span(transaction.contexts.trace.span_id, table_name: table_name)

	assert nil ==
	SpanStorage.get_root_span(transaction.contexts.trace.span_id, table_name: table_name)
	assert SpanStorage.get_root_span(transaction.contexts.trace.span_id, table_name: table_name) == nil

	assert String.length(trace_id) == 32, "Expected trace_id to be 32 characters long #{trace_id}"
	assert byte_size(trace_id) == 32, "Expected trace_id to be 32 characters long #{trace_id}"

		assert String.match?(trace_id, ~r/^[a-f0-9]{32}$/),
		"Expected trace_id to be a lowercase hex string"

Add SpanProcessor for OpenTelemetry #875

Are you sure you want to change the base?

Add SpanProcessor for OpenTelemetry #875

Conversation

solnic commented Mar 14, 2025 • edited Loading

solnic commented Mar 14, 2025

whatyouhide commented Mar 17, 2025

sl0thentr0py commented Mar 17, 2025 • edited Loading

solnic commented Mar 18, 2025

solnic commented Mar 18, 2025

whatyouhide commented Mar 18, 2025

solnic commented Mar 18, 2025

sl0thentr0py commented Mar 18, 2025

solnic commented Mar 19, 2025

sl0thentr0py commented Mar 25, 2025

sl0thentr0py left a comment

Choose a reason for hiding this comment

whatyouhide left a comment

Choose a reason for hiding this comment

whatyouhide commented Apr 3, 2025

solnic commented Apr 3, 2025

solnic commented Apr 3, 2025

sergiotapia commented May 1, 2025

whatyouhide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sl0thentr0py commented May 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whatyouhide commented May 14, 2025

solnic commented Mar 14, 2025 •

edited

Loading

sl0thentr0py commented Mar 17, 2025 •

edited

Loading