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

Record dropped spans #4172

Merged
merged 25 commits into from
Jul 23, 2024
Merged

Record dropped spans #4172

merged 25 commits into from
Jul 23, 2024

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Jul 16, 2024

📜 Description

💡 Motivation and Context

Closes #4156

💚 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

Please check if these are all instances where we need to record dropped spans, as they are a bit different from Flutter/Java and i'm not too familiar with the Cocoa SDK. 🙇

Copy link

github-actions bot commented Jul 16, 2024

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

Generated by 🚫 dangerJS against b125bd4

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

First pass. I will wait until the PR is open to review it again.

Sources/Sentry/SentryClient.m Show resolved Hide resolved
Sources/Sentry/SentryDataCategoryMapper.m Outdated Show resolved Hide resolved
Sources/Sentry/include/SentryDataCategory.h Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 97.95918% with 6 lines in your changes missing coverage. Please review.

Project coverage is 91.411%. Comparing base (10f96ae) to head (b125bd4).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4172       +/-   ##
=============================================
+ Coverage   91.358%   91.411%   +0.052%     
=============================================
  Files          606       606               
  Lines        48373     48658      +285     
  Branches     17468     17559       +91     
=============================================
+ Hits         44193     44479      +286     
+ Misses        4087      4086        -1     
  Partials        93        93               
Files Coverage Δ
SentryTestUtils/TestClient.swift 84.821% <100.000%> (+0.417%) ⬆️
SentryTestUtils/TestTransport.swift 92.857% <100.000%> (+1.948%) ⬆️
Sources/Sentry/SentryClient.m 98.574% <100.000%> (+0.109%) ⬆️
Sources/Sentry/SentryDataCategoryMapper.m 98.648% <100.000%> (+1.505%) ⬆️
Sources/Sentry/SentryEnvelopeRateLimit.m 96.153% <100.000%> (ø)
Sources/Sentry/SentryFileManager.m 93.281% <100.000%> (ø)
Sources/Sentry/SentryHub.m 98.378% <100.000%> (+0.013%) ⬆️
Sources/Sentry/SentrySpotlightTransport.m 97.058% <ø> (ø)
Sources/Sentry/SentryTransportAdapter.m 100.000% <100.000%> (ø)
...s/SentryTests/Helper/TestFileManagerDelegate.swift 100.000% <100.000%> (ø)
... and 7 more

... and 4 files with indirect coverage changes


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 10f96ae...b125bd4. Read the comment docs.

@denrase denrase marked this pull request as ready for review July 17, 2024 12:02
@denrase denrase requested a review from brustolin July 17, 2024 12:02
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

Thanks for this. Looks good.

Sources/Sentry/SentryHttpTransport.m Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Jul 22, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1221.29 ms 1224.27 ms 2.97 ms
Size 21.58 KiB 683.64 KiB 662.06 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
699d76f 1233.96 ms 1251.32 ms 17.36 ms
1ee3d54 1241.80 ms 1250.67 ms 8.87 ms
7c5d161 1224.38 ms 1249.66 ms 25.28 ms
69d8759 1229.88 ms 1240.45 ms 10.57 ms
0ffe199 1209.51 ms 1223.94 ms 14.43 ms
2e3ef92 1226.51 ms 1241.21 ms 14.70 ms
d9cd5f1 1234.14 ms 1257.69 ms 23.54 ms
e84bc3f 1201.49 ms 1232.82 ms 31.33 ms
9d28681 1230.08 ms 1248.55 ms 18.47 ms
08e12bc 1204.51 ms 1215.98 ms 11.47 ms

App size

Revision Plain With Sentry Diff
699d76f 21.58 KiB 631.82 KiB 610.24 KiB
1ee3d54 21.58 KiB 571.76 KiB 550.18 KiB
7c5d161 20.76 KiB 414.44 KiB 393.68 KiB
69d8759 20.76 KiB 393.05 KiB 372.29 KiB
0ffe199 20.76 KiB 436.50 KiB 415.74 KiB
2e3ef92 21.58 KiB 671.89 KiB 650.31 KiB
d9cd5f1 21.58 KiB 544.73 KiB 523.14 KiB
e84bc3f 20.76 KiB 434.72 KiB 413.96 KiB
9d28681 20.76 KiB 399.19 KiB 378.43 KiB
08e12bc 21.58 KiB 539.88 KiB 518.30 KiB

Previous results on branch: feat/report-dropped-spans

Startup times

Revision Plain With Sentry Diff
e1edec5 1228.70 ms 1238.67 ms 9.98 ms

App size

Revision Plain With Sentry Diff
e1edec5 21.58 KiB 683.23 KiB 661.65 KiB

@denrase
Copy link
Collaborator Author

denrase commented Jul 22, 2024

@brustolin Do you have an idea why the benchmarking task is failing? Building with fastlane seems to be the issue?

@brustolin
Copy link
Contributor

brustolin commented Jul 22, 2024

Do you have an idea why the benchmarking task is failing?

It seems flaky. I don't think it's related to the PR

@brustolin
Copy link
Contributor

@denrase What's keeping us from merging this?

@denrase
Copy link
Collaborator Author

denrase commented Jul 23, 2024

@brustolin Just the CI, thx for clarification 🙇

@denrase denrase merged commit 2a769ba into main Jul 23, 2024
67 checks passed
@denrase denrase deleted the feat/report-dropped-spans branch July 23, 2024 10:06
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.

Report dropped spans in client reports
3 participants