Skip to content

Add new sentry-opentelemetry gem#1948

Merged
sl0thentr0py merged 12 commits intomasterfrom
neel/otel-gem
Dec 1, 2022
Merged

Add new sentry-opentelemetry gem#1948
sl0thentr0py merged 12 commits intomasterfrom
neel/otel-gem

Conversation

@sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Nov 23, 2022

Summary

  • new gem sentry-opentelemetry to be used as described here
  • all sentry instrumentation will be disabled by a new config.instrumeneter option so that sentry and otel do not step on each other's toes
  • the 2 main things are
    • a SpanProcessor that hooks into on_start/on_finish whenever OpenTelemetry starts/finishes a span. This in turn creates sentry transactions/spans as necessary and keeps track of them in an internal span_map hash.
    • a Propagator to make sure distributed tracing and dynamic sampling work by hooking into inject/extract for incoming/outgoing sentry-trace/baggage header management.

Spec

https://develop.sentry.dev/sdk/performance/opentelemetry/

#skip-changelog

@sl0thentr0py sl0thentr0py requested a review from st0012 November 23, 2022 16:19
@sl0thentr0py sl0thentr0py changed the base branch from master to neel/add-tx-contexts November 23, 2022 16:19
Base automatically changed from neel/add-tx-contexts to master November 29, 2022 13:49
@sl0thentr0py
Copy link
Member Author

@st0012 do you still want to review this separately or are you ok with us shipping this?

@st0012
Copy link
Contributor

st0012 commented Nov 29, 2022

@sl0thentr0py I want to review this separately because I was focusing on other changes in my first pass.
I'm leaving some comments now.

@sl0thentr0py
Copy link
Member Author

ok, no rush then! just wanted to confirm

attr_reader :span_map

def initialize
@span_map = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may store configuration.logger and configuration.instrumenter here so we don't need to perform configuration lookup repeatedly.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • if we do this, the precedence order of initialization comes back because you need access to the instantiated Sentry.configuration object
  • eventually we might do all the otel config automatically ourselves and then we can fix all precedence problems in our setup, but we don't want to make assumptions on user setups right now

for now, I will just kill the logger statements since they were mostly for development and keep the other stuff as is.

@sl0thentr0py
Copy link
Member Author

@st0012 bump, is there something more that you want to discuss here?
I'm going on vacation next week, and we want to release this in the next few days so there's some time for bug reports etc before I leave.

Once again, this is a standalone gem and it does not have to be the perfect implementation, the main idea is

  • to start ingesting otel spans into sentry in the simplest and most low-friction way possible
  • ensure distributed tracing works (via propagator)
  • collect feedback on improving SDK/data/product experience and iterate on it later

We have thought through some of the trade-offs and are aware of some possible problems, your feedback is of course very welcome.

Copy link
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Yeah I think we can test it first. Just one more thing: do you want to add it to the top-level` readme too?

@sl0thentr0py
Copy link
Member Author

docs PR merged, will be live soon
getsentry/sentry-docs#5856

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2022

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.41%. Comparing base (3567021) to head (fb5718a).
⚠️ Report is 475 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1948      +/-   ##
==========================================
+ Coverage   98.35%   98.41%   +0.05%     
==========================================
  Files         151      156       +5     
  Lines        9450     9793     +343     
==========================================
+ Hits         9295     9638     +343     
  Misses        155      155              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants