Skip to content
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

Support for Traces/Transactions via Opentelemetry #784

Open
wants to merge 53 commits into
base: master
Choose a base branch
from

Conversation

solnic
Copy link
Collaborator

@solnic solnic commented Sep 4, 2024

This adds support for Traces via Opentelemetry integration, handled by our custom Sentry.Opentelemetry.SpanProcessor which accumulates spans and turns them into hierarchical transactions that are then sent to Sentry.

For this to work the following deps must be added to an app:

{:opentelemetry, "~> 1.4"},
{:opentelemetry_api, "~> 1.3"}

Furthermore, Opentelemetry needs to be configured with our SpanProcessor:

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

The rest will automagically work.

Supported opentelemetry packages

This initial implementation supports the following Opentelemetry packages:

  • opentelemetry_phoenix
  • opentelemetry_bandit
  • opentelemetry_ecto
  • opentelemetry_oban

Considerations

  • Should we add new deps as sentry deps instead of having people add them manually to their mix.exs?
  • Would it make sense to try to pre-configure opentelemetry automatically?
  • Which other opentelemetry_* packages do we want to support initially?
  • Should we support top-level ecto events that occur when ie Ecto tries to create migrations table, or checks migrations? (I think we should ignore them, FWIW)

@solnic solnic force-pushed the solnic/support-for-transactions-via-otel branch 5 times, most recently from 6f8f1ea to 1478508 Compare September 20, 2024 10:12
@solnic solnic force-pushed the solnic/support-for-transactions-via-otel branch 4 times, most recently from e8a40bb to cb77932 Compare September 27, 2024 13:16
@solnic solnic changed the title [WIP] Support for Transactions/Spans via Opentelemetry Span Processor Support for Transactions/Spans via Opentelemetry Span Processor Sep 27, 2024
@solnic solnic marked this pull request as ready for review September 27, 2024 14:16
@solnic solnic changed the title Support for Transactions/Spans via Opentelemetry Span Processor Support for Traces via Opentelemetry Sep 27, 2024
@solnic solnic changed the title Support for Traces via Opentelemetry Support for Traces/Transactions via Opentelemetry Sep 27, 2024
@solnic solnic mentioned this pull request Oct 8, 2024
@solnic solnic force-pushed the solnic/support-for-transactions-via-otel branch from cb77932 to 93579f3 Compare October 14, 2024 10:05
@whatyouhide whatyouhide added this to the 10.8.0 milestone Oct 22, 2024
@whatyouhide
Copy link
Collaborator

@solnic can you rebase this one now that #797 has been merged? Gonna make it easier to review I think 😄

@solnic solnic force-pushed the solnic/support-for-transactions-via-otel branch 2 times, most recently from 87fe610 to 563e416 Compare October 23, 2024 09:16
@solnic
Copy link
Collaborator Author

solnic commented Oct 23, 2024

@solnic can you rebase this one now that #797 has been merged? Gonna make it easier to review I think 😄

@whatyouhide done!

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
lib/sentry.ex Outdated Show resolved Hide resolved
lib/sentry/opentelemetry/span_processor.ex Outdated Show resolved Hide resolved
lib/sentry/opentelemetry/span_storage.ex Outdated Show resolved Hide resolved
mix.exs Outdated Show resolved Hide resolved
@solnic solnic force-pushed the solnic/support-for-transactions-via-otel branch from 08cec4c to e77a601 Compare October 28, 2024 09:15
solnic and others added 23 commits December 27, 2024 11:57
Turns out `optional` deps in mix.exs work differently in
older Elixir, as it would not load opentelemetry deps which
made our tests fail because opentelemetry setup was
not loaded.
* Add environment to transaction

* Make root span the transaction

* Generic transactions spans parsing
- Use the worker name as the description and transaction name instead of "{worker} process"
- Use "queue.process" as the op - this is according to the docs https://develop.sentry.dev/sdk/telemetry/traces/modules/queues/

Maybe this could be improved in the opentelemetry_oban package?
Co-authored-by: Dan Schultzer <[email protected]>
@solnic solnic force-pushed the solnic/support-for-transactions-via-otel branch from 8574f4c to a808377 Compare December 27, 2024 11:58
@solnic
Copy link
Collaborator Author

solnic commented Dec 27, 2024

I've split this into #842 and #843 /cc @whatyouhide

@solnic solnic mentioned this pull request Dec 27, 2024
@grantwest
Copy link

I've split this into #842 and #843 /cc @whatyouhide

Does that mean this PR will be closed without merging?

@solnic
Copy link
Collaborator Author

solnic commented Jan 7, 2025

I've split this into #842 and #843 /cc @whatyouhide

Does that mean this PR will be closed without merging?

Depending on which one is simpler to review, to be honest. I can of course rework this one on top of #842 but this is the 50th comment here so maybe we'd benefit from a cleaner slate. WDYT @whatyouhide?

@solnic solnic mentioned this pull request Jan 27, 2025
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.

7 participants