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

Implement sentry-trace propagation #1446

Merged
merged 4 commits into from May 21, 2021
Merged

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented May 20, 2021

This implements and closes #1445

Summary

The SDK will insert the sentry-trace to outgoing requests made with Net::HTTP. Its value would look like d827317d25d5420aa3aa97a0257db998-57757614642bdff5-1.

If the receiver service also uses Sentry and the SDK supports performance monitoring, its tracing event will be connected with the sender application's.

Example:

connect sentry trace

This feature is activated by default. But users can use the new config.propagate_traces config option to disable it.

@st0012 st0012 self-assigned this May 20, 2021
@st0012 st0012 added this to In progress in 4.x via automation May 20, 2021
@st0012 st0012 added this to the 4.5.0 milestone May 20, 2021
@st0012 st0012 requested a review from rhcarvalho May 20, 2021 10:25
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2021

Codecov Report

Merging #1446 (be751a3) into master (5ce32c1) will increase coverage by 0.62%.
The diff coverage is 100.00%.

❗ Current head be751a3 differs from pull request most recent head 4b68d22. Consider uploading reports for the commit 4b68d22 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1446      +/-   ##
==========================================
+ Coverage   98.22%   98.85%   +0.62%     
==========================================
  Files         213      118      -95     
  Lines       10034     6261    -3773     
==========================================
- Hits         9856     6189    -3667     
+ Misses        178       72     -106     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry-ruby.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/configuration.rb 97.95% <100.00%> (+0.02%) ⬆️
sentry-ruby/lib/sentry/hub.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/net/http.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/hub_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/net/http_spec.rb 100.00% <100.00%> (ø)
...raven/spec/raven/integrations/rack_timeout_spec.rb
...ations/rails/overrides/debug_exceptions_catcher.rb
sentry-raven/spec/raven/utils/request_id_spec.rb
...ven/breadcrumbs/active_support_breadcrumbs_spec.rb
... and 91 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ce32c1...4b68d22. Read the comment docs.

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

I think we just need a method name change to match the SDK guidelines and we're good to merge.

Thanks @st0012 !

sentry-ruby/lib/sentry/net/http.rb Outdated Show resolved Hide resolved
sentry-ruby/lib/sentry/net/http.rb Show resolved Hide resolved
4.x automation moved this from In progress to Review in progress May 20, 2021
def generate_sentry_trace(span = nil)
return unless current_client.configuration.propagate_traces

span ||= current_scope.get_span
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rhcarvalho let's move the discussion here.
Another reason I choose Hub instead of Client is because in Hub we can also access the current scope.

Consider that this method needs to access different both Client and Scope, I still think Hub is currently the best place to have this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

#1446 (comment)
I'd say we don't need the Hub because it is only used to get the current scope when the span is nil, but we never call it with a nil span.

I have the impression this line is unnecessary, isn't it? The only non-test place where this method is called is set_sentry_trace_header and we never call with a nil span.

So my thought was that this didn't necessarily need to access the current scope.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought being able to read the span from current scope is a hard requirement when I checked sentry-python's implementation.

https://github.com/getsentry/sentry-python/blob/4c09f3203d6d19789c6fa729a2e46557ad4ea913/sentry_sdk/hub.py#L692-L694

But if it's not, I'm happy to remove it and move this method to Client.

@st0012 st0012 force-pushed the implement-sentry-trace-propagation branch from b996870 to 080de9e Compare May 21, 2021 10:00
@st0012 st0012 requested a review from rhcarvalho May 21, 2021 10:02
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Thanks @st0012 !

4.x automation moved this from Review in progress to Reviewer approved May 21, 2021
@st0012
Copy link
Collaborator Author

st0012 commented May 21, 2021

@rhcarvalho thanks for the review!

@st0012 st0012 merged commit e4b6d66 into master May 21, 2021
@st0012 st0012 deleted the implement-sentry-trace-propagation branch May 21, 2021 13:01
4.x automation moved this from Reviewer approved to Done May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.x
  
Done
Development

Successfully merging this pull request may close these issues.

Automatic trace propagation in outgoing HTTP requests
3 participants