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

Respect incoming parent sampled decision when continuing a trace #2311

Merged
merged 3 commits into from
Oct 20, 2022

Conversation

adinauer
Copy link
Member

📜 Description

When introducing baggage we ignored the sampled decision coming in via sentry-trace. This caused incoming sampled=false to not be honored.

💡 Motivation and Context

Fixes #2308

💚 How did you test it?

Unit Tests

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 292.60 ms 326.21 ms 33.61 ms
Size 1.73 MiB 2.32 MiB 607.12 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
2c5f172 289.18 ms 307.56 ms 18.38 ms
4ca1d7b 331.33 ms 335.78 ms 4.45 ms
54cebc8 300.86 ms 341.43 ms 40.57 ms
3d89dea 322.38 ms 350.82 ms 28.45 ms
221a5df 350.61 ms 391.58 ms 40.97 ms
1e4690d 354.69 ms 387.88 ms 33.19 ms
4dd88fe 306.88 ms 391.58 ms 84.70 ms
2c5f172 351.18 ms 373.52 ms 22.34 ms
2c5f172 310.20 ms 357.16 ms 46.96 ms
d4087ee 278.00 ms 313.86 ms 35.86 ms

App size

Revision Plain With Sentry Diff
2c5f172 1.73 MiB 2.29 MiB 580.10 KiB
4ca1d7b 1.73 MiB 2.29 MiB 579.88 KiB
54cebc8 1.73 MiB 2.29 MiB 579.43 KiB
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB
221a5df 1.73 MiB 2.29 MiB 581.39 KiB
1e4690d 1.74 MiB 2.33 MiB 604.92 KiB
4dd88fe 1.73 MiB 2.29 MiB 579.50 KiB
2c5f172 1.73 MiB 2.29 MiB 580.10 KiB
2c5f172 1.73 MiB 2.29 MiB 580.10 KiB
d4087ee 1.73 MiB 2.29 MiB 579.50 KiB

Previous results on branch: fix/take-parent-sampled-from-sentry-trace-header

Startup times

Revision Plain With Sentry Diff
284caeb 277.96 ms 340.65 ms 62.69 ms

App size

Revision Plain With Sentry Diff
284caeb 1.73 MiB 2.32 MiB 607.34 KiB

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

Codecov Report

Base: 80.09% // Head: 80.17% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (78e438f) compared to base (f50fb16).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2311      +/-   ##
============================================
+ Coverage     80.09%   80.17%   +0.07%     
- Complexity     3431     3435       +4     
============================================
  Files           242      242              
  Lines         12756    12757       +1     
  Branches       1703     1704       +1     
============================================
+ Hits          10217    10228      +11     
+ Misses         1890     1879      -11     
- Partials        649      650       +1     
Impacted Files Coverage Δ
...ry/src/main/java/io/sentry/TransactionContext.java 71.15% <66.66%> (+10.36%) ⬆️
sentry/src/main/java/io/sentry/Baggage.java 85.80% <0.00%> (+3.22%) ⬆️

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.

@romtsn
Copy link
Member

romtsn commented Oct 19, 2022

@stefanosiano there's a flakey test which I've also encountered in another PR - could you perhaps take a look at it when you got time? https://github.com/getsentry/sentry-java/actions/runs/3281479762/jobs/5403550895

@adinauer adinauer merged commit 418a49e into main Oct 20, 2022
@adinauer adinauer deleted the fix/take-parent-sampled-from-sentry-trace-header branch October 20, 2022 07:52
@@ -80,10 +80,11 @@ public final class TransactionContext extends SpanContext {
baggage.freeze();

Double sampleRate = baggage.getSampleRateDouble();
Boolean sampled = parentSampled != null ? parentSampled.booleanValue() : true;

Choose a reason for hiding this comment

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

Why parentSampled is null should be sampled ?

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.

Why baggage header override parent sampling decision
5 participants