Skip to content

Conversation

@brustolin
Copy link
Contributor

📜 Description

Adds sampled flag to all trace envelope headers

💡 Motivation and Context

closes #3201

💚 How did you test it?

Unit tests

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Sep 18, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 0205f00

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #3286 (507c12a) into main (e758449) will increase coverage by 0.045%.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3286       +/-   ##
=============================================
+ Coverage   89.169%   89.215%   +0.045%     
=============================================
  Files          502       502               
  Lines        54263     54270        +7     
  Branches     19480     19488        +8     
=============================================
+ Hits         48386     48417       +31     
+ Misses        5016      4882      -134     
- Partials       861       971      +110     
Files Changed Coverage
Sources/Sentry/SentryBaggage.m 100.000%
Sources/Sentry/SentryPropagationContext.m 100.000%
Sources/Sentry/SentryTraceContext.m 100.000%
.../SentryTests/Helper/SentrySerializationTests.swift 100.000%
...sts/SentryTests/Protocol/SentryEnvelopeTests.swift 100.000%
...s/SentryTests/Transaction/SentryBaggageTests.swift 100.000%
...entryTests/Transaction/SentryTraceStateTests.swift 100.000%

Continue to review full report in Codecov by Sentry.

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

@github-actions
Copy link
Contributor

github-actions bot commented Sep 18, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1254.94 ms 1266.38 ms 11.44 ms
Size 22.85 KiB 408.88 KiB 386.03 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
881a955 1254.14 ms 1268.43 ms 14.29 ms
1bf8571 1250.96 ms 1255.36 ms 4.40 ms
736495a 1245.16 ms 1254.42 ms 9.26 ms
d257eb9 1206.98 ms 1227.50 ms 20.52 ms
effc81c 1253.71 ms 1256.16 ms 2.45 ms
adca747 1199.37 ms 1215.49 ms 16.12 ms
a2af9fa 1236.62 ms 1253.12 ms 16.50 ms
90d17d3 1261.18 ms 1278.18 ms 17.00 ms
f8fc36d 1226.31 ms 1247.80 ms 21.49 ms
7cd187e 1196.51 ms 1226.04 ms 29.53 ms

App size

Revision Plain With Sentry Diff
881a955 22.85 KiB 407.63 KiB 384.79 KiB
1bf8571 20.76 KiB 437.12 KiB 416.36 KiB
736495a 20.76 KiB 435.22 KiB 414.46 KiB
d257eb9 20.76 KiB 433.22 KiB 412.46 KiB
effc81c 20.76 KiB 433.18 KiB 412.42 KiB
adca747 20.76 KiB 401.36 KiB 380.60 KiB
a2af9fa 20.76 KiB 432.87 KiB 412.11 KiB
90d17d3 20.76 KiB 432.17 KiB 411.41 KiB
f8fc36d 20.76 KiB 419.70 KiB 398.94 KiB
7cd187e 20.76 KiB 401.66 KiB 380.90 KiB

Previous results on branch: feat/sampling-decision-trace-envelope

Startup times

Revision Plain With Sentry Diff
b67b375 1242.42 ms 1249.66 ms 7.24 ms
a49138a 1250.61 ms 1261.72 ms 11.11 ms

App size

Revision Plain With Sentry Diff
b67b375 22.85 KiB 407.94 KiB 385.09 KiB
a49138a 22.85 KiB 407.88 KiB 385.03 KiB

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Changes look fine. Though I wonder if an NSNumber encapsulating a boolean wouldn't be more appropriate than a string.

If we want to leave it as a string, we should consider using defined constants for the values.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Updates look good, thanks!

@brustolin brustolin merged commit 72c8d84 into main Sep 21, 2023
@brustolin brustolin deleted the feat/sampling-decision-trace-envelope branch September 21, 2023 08:48
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.

Add Sampling Decision to Trace Envelope Header

4 participants