-
-
Notifications
You must be signed in to change notification settings - Fork 515
Propagated sampling rates #2671
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
Conversation
fa9fe50
to
e1b1e74
Compare
e1b1e74
to
e8da627
Compare
e8da627
to
206704f
Compare
f054365
to
c720d30
Compare
c720d30
to
3581ae9
Compare
3581ae9
to
0fdbceb
Compare
2706368
to
763c8ad
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2671 +/- ##
==========================================
+ Coverage 97.39% 97.44% +0.04%
==========================================
Files 135 136 +1
Lines 5229 5315 +86
==========================================
+ Hits 5093 5179 +86
Misses 136 136
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic looks perfect, nice work!
if we can avoid deduplication it would be great, but maybe I missed a reason why transaction needs its own logic. We will remove Transaction.from_sentry_trace
completely in the major either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some suggestions for the specs
sentry-ruby/spec/sentry/transactions/sample_rand_propagation_spec.rb
Outdated
Show resolved
Hide resolved
sentry-ruby/spec/sentry/transactions/sample_rand_propagation_spec.rb
Outdated
Show resolved
Hide resolved
7a2d646
to
5ad2318
Compare
4ed23c3
to
db1f2d8
Compare
d85621e
to
360d3bf
Compare
dsc_metadata | ||
end | ||
|
||
def get_http_server_transactions_with_headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry if I'm blind but where are we using this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh sorry about that, messed up rebasing and unused methods kept reappearing, it's removed now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, but then I wanted us to check the incoming request headers too by actually using this method, not removing it..
anyway it's just a spec so don't care that much, feel free to add it or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sl0thentr0py OK added expectations for proper headers and stuff via db7da2e
360d3bf
to
f1881f5
Compare
3fc0e46
to
db7da2e
Compare
This ensures that we follow the spec as described here https://develop.sentry.dev/sdk/telemetry/traces/#propagated-random-value