Skip to content

Add config.instrumenter to switch between sentry and otel instrumentation - #1944

Merged
sl0thentr0py merged 1 commit into
masterfrom
neel/config-instrumenter
Nov 29, 2022
Merged

Add config.instrumenter to switch between sentry and otel instrumentation#1944
sl0thentr0py merged 1 commit into
masterfrom
neel/config-instrumenter

Conversation

@sl0thentr0py

Copy link
Copy Markdown
Member

The new config.instrumenter will act as a top-level switch to disable on sentry transactions/spans when we are in :otel mode.

  • Disable rails subscribers when instrumenter is not :sentry
  • Noop top level start_transaction and with_child_span APIs when the instrumenter don't match with the currently active instrumenter (defaults to :sentry)

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Base: 98.33% // Head: 98.33% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (248ad25) compared to base (5bfdf80).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@ Coverage Diff @@## master #1944 +/- ##
=======================================
Coverage 98.33% 98.33% =======================================
Files 151 151 Lines 9306 9369 +63 =======================================
+ Hits 9151 9213 +62 - Misses 155 156 +1 
Impacted FilesCoverage Δ
sentry-rails/lib/sentry/rails/railtie.rb97.33% <100.00%> (ø)
sentry-rails/spec/sentry/rails/tracing_spec.rb99.40% <100.00%> (+0.03%)⬆️
sentry-ruby/lib/sentry/configuration.rb98.47% <100.00%> (+0.03%)⬆️
sentry-ruby/lib/sentry/hub.rb100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/configuration_spec.rb100.00% <100.00%> (ø)
sentry-ruby/spec/sentry_spec.rb99.80% <100.00%> (+0.01%)⬆️
sentry-ruby/lib/sentry/breadcrumb.rb96.29% <0.00%> (-3.71%)⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@st0012

Copy link
Copy Markdown
Contributor

Should this be applied to sentry-sidekiq, sentry-resque, and sentry-delayed_job as well?
If the answer is yes, can we encapsulate Sentry.configuration.instrumenter == :sentry with something like Sentry.configuration.native_instrumenter? or Sentry.configuration.default_instrumenter??

@sl0thentr0py

Copy link
Copy Markdown
MemberAuthor

@st0012 so the idea was to make this config as non-interfering as possible, since we don't know the future of this feature.

I don't want to add top-level apis we commit to for now, so the idea was to no-op the top-level hub start_transaction/with_child_span apis and also not to change every callsite. All the other gems/integrations should remain backwards compat with minimal change.

I added the rails check because of the span nil issue, which is fine in this case I think to avoid all the checks.

Potential users of sentry-opentelemetry will also ideally not add sentry-rails|sidekiq|delayed_job|resque since they will be using the otel sdk for instrumentation, so this is also another argument be made for leaving the other integrations untouched.

@AbhiPrasad

Copy link
Copy Markdown
Contributor

Potential users of sentry-opentelemetry will also ideally not add sentry-rails|sidekiq|delayed_job|resque since they will be using the otel sdk for instrumentation, so this is also another argument be made for leaving the other integrations untouched.

Do we do error monitoring/add breadcrumbs via sentry-rails|sidekiq|delayed_job|resque? If yes, this will mean that they have to use these integrations anyway.

@sl0thentr0py

Copy link
Copy Markdown
MemberAuthor

hmm yea, nvm you're right, ignore that second argument
the other argument about not changing the call-sites still holds.

@st0012

Copy link
Copy Markdown
Contributor

@sl0thentr0py Can you also add tests to those integrations to make sure they don't raise errors when the instrumenter is otel?

@sl0thentr0py
sl0thentr0pyforce-pushed the neel/config-instrumenter branch from 248ad25 to 9097a81CompareNovember 28, 2022 14:01
@sl0thentr0py
sl0thentr0pyforce-pushed the neel/config-instrumenter branch from 9097a81 to 09f1c1bCompareNovember 28, 2022 14:25
@sl0thentr0py

Copy link
Copy Markdown
MemberAuthor

added config/otel specs to other gems

@sl0thentr0py
sl0thentr0py merged commit a62fb2d into masterNov 29, 2022
@sl0thentr0py
sl0thentr0py deleted the neel/config-instrumenter branch November 29, 2022 12:48
Sign up for freeto 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.

4 participants

@sl0thentr0py@codecov-commenter@st0012@AbhiPrasad