Skip to content

Add SpanProcessor for OpenTelemetry #875

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.

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

Merged
merged 48 commits into from
May 21, 2025
Merged

Conversation

solnic
Copy link
Collaborator

@solnic solnic commented Mar 14, 2025

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:

Screenshot 2025-03-19 at 11 32 54 Screenshot 2025-03-19 at 11 31 53 Screenshot 2025-03-19 at 11 33 03 Screenshot 2025-03-19 at 11 29 03

@solnic solnic linked an issue Mar 14, 2025 that may be closed by this pull request
@solnic solnic force-pushed the 874-add-spanprocessor-for-otel branch from 0ebd132 to dee9540 Compare March 14, 2025 11:17
@solnic
Copy link
Collaborator Author

solnic commented Mar 14, 2025

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
Copy link
Collaborator

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

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Mar 17, 2025

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
Copy link
Collaborator Author

solnic commented Mar 18, 2025

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
Copy link
Collaborator Author

solnic commented Mar 18, 2025

@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 🙃

@solnic solnic mentioned this pull request Mar 18, 2025
@whatyouhide
Copy link
Collaborator

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

@solnic
Copy link
Collaborator Author

solnic commented Mar 18, 2025

@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
Copy link
Member

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 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 solnic marked this pull request as ready for review March 19, 2025 12:59
@solnic
Copy link
Collaborator Author

solnic commented Mar 19, 2025

@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
Copy link
Member

okay I'm taking a preliminary look now

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

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

Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

some suggestions, looks very good otherwise!

Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

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!

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

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

@solnic
Copy link
Collaborator Author

solnic commented Apr 3, 2025

@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 solnic force-pushed the 874-add-spanprocessor-for-otel branch from c9152c6 to 1654724 Compare April 3, 2025 10:44
@solnic
Copy link
Collaborator Author

solnic commented Apr 3, 2025

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

@solnic solnic force-pushed the 874-add-spanprocessor-for-otel branch from 01585a8 to 5e8fc99 Compare April 3, 2025 11:07
@solnic solnic requested a review from sl0thentr0py May 20, 2025 14:11
@solnic solnic force-pushed the 874-add-spanprocessor-for-otel branch from 48fbd08 to f1183b3 Compare May 20, 2025 14:47
Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

lgtm!! :shipit:
and thank you for the feature

@solnic solnic dismissed whatyouhide’s stale review May 21, 2025 06:22

The remaining feedback will be addressed in follow-up PRs as this one has become too big with over 140 comments.

@solnic solnic merged commit 9887bd0 into master May 21, 2025
5 checks passed
@solnic solnic deleted the 874-add-spanprocessor-for-otel branch May 21, 2025 06:22
solnic added a commit that referenced this pull request May 22, 2025
solnic added a commit that referenced this pull request May 26, 2025
@jwaldrip
Copy link

jwaldrip commented Jun 3, 2025

@solnic Why did this get reverted? Is there no OpenTelemetry span support now?

@solnic
Copy link
Collaborator Author

solnic commented Jun 4, 2025

@jwaldrip we had to revert to push 10.10.0 with OTP 28 fixes. It'll be restored soon with improved Sampler that's the missing piece for finally releasing it.

solnic added a commit that referenced this pull request Jun 4, 2025
solnic added a commit that referenced this pull request Jun 10, 2025
solnic added a commit that referenced this pull request Jun 17, 2025
* Revert "Revert "Add SpanProcessor for OpenTelemetry (#875)""

This reverts commit 2ced90e.

* Better span sampler (#903)

* Better span sampler

This updates OpenTelemetry.Sampler so that it uses
`traces_sample_rate` for sampling spans before they
get sent to the span processor.

This way we're not processing spans when they are
dropped during sampling, which was previously the
case as the Client was responsible for sampling
just before attemping to send a transaction.

* Enhance sampling logic to record discarded transactions

* Clarify trace-level sampling decision-making

Previously it incorrectly referred to "parent" where
in reality we're dealing with spans from the same
trace and respect sampling decisions that were
already made for a given trace.

This is not the same thing as parent/child spans
that we're handling in the SpanProcessor as this
type of relationship is established post-sampling.

* Remove unnecessary sleep calls in sampler tests

* Fix flaky test

* Clean up make_sampling_decision

* Simplify client reports for dropped transactions

* Update traces_sample_rate default value to be nil

* Account for `nil` sample_rate when sampling

* Use put_test_config

* Update traces_sample_rate spec to allow nil value

* Support for traces_sampler option (#910)

* Add support for `traces_sampler` config option

* Update docs

* Remove unnecessary attribute merging

* Call traces_sampler only for root spans

* Reuse sampling decision logic

* Add test for handling invalid traces_sampler return values

* Add tests for SamplingContext access functions

* Add basic integration tests for OpenTelemetry (#912)

* Update CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SpanProcessor for OTel
7 participants