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

fix: Flush directly after capturing envelopes #3915

Merged
merged 19 commits into from May 10, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

Calling flush directly after capturing any envelope did not ensure that the SDK flushes the captured envelope. This is fixed now.

💡 Motivation and Context

This came up when fixing UI tests for capturing the transaction on a background thread: #3892. I think this caused the unit tests for flush to be flaky. I enabled them again. If they are again flaky, we have to disable them again.

💚 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

Calling flush directly after capturing any envelope did not ensure that
the SDK flushes the captured envelope. This is fixed now.
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.868%. Comparing base (8cbdf25) to head (6f6ca7c).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3915       +/-   ##
=============================================
+ Coverage   90.753%   90.868%   +0.115%     
=============================================
  Files          593       594        +1     
  Lines        46038     46061       +23     
  Branches     16418     16432       +14     
=============================================
+ Hits         41781     41855       +74     
+ Misses        4187      4136       -51     
  Partials        70        70               
Files Coverage Δ
SentryTestUtils/ClearTestState.swift 100.000% <100.000%> (ø)
SentryTestUtils/SentryLogExtensions.swift 100.000% <100.000%> (ø)
Sources/Sentry/SentryHttpTransport.m 98.181% <100.000%> (+0.022%) ⬆️
Sources/Sentry/SentryTransportFactory.m 100.000% <100.000%> (ø)
...Tests/Integrations/ANR/SentryANRTrackerTests.swift 85.185% <100.000%> (ø)
...ns/Performance/SentryPerformanceTrackerTests.swift 100.000% <100.000%> (ø)
...tryTests/Networking/SentryHttpTransportTests.swift 99.667% <100.000%> (+3.069%) ⬆️
Tests/SentryTests/SentryHubTests.swift 98.850% <100.000%> (ø)
...Tests/TestUtils/TestConncurrentModifications.swift 100.000% <100.000%> (ø)

... and 8 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 8cbdf25...6f6ca7c. Read the comment docs.

Copy link

github-actions bot commented Apr 29, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1245.02 ms 1253.78 ms 8.76 ms
Size 21.58 KiB 616.86 KiB 595.28 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7fb7afb 1235.00 ms 1256.81 ms 21.81 ms
a9103fe 1265.47 ms 1267.37 ms 1.90 ms
725565a 1234.59 ms 1250.02 ms 15.43 ms
2b19b82 1212.94 ms 1252.84 ms 39.90 ms
965db8a 1212.94 ms 1233.41 ms 20.47 ms
fc163f5 1198.05 ms 1227.76 ms 29.71 ms
b16d18c 1245.60 ms 1250.94 ms 5.34 ms
1734d1b 1198.69 ms 1221.62 ms 22.93 ms
addcf42 1247.33 ms 1253.58 ms 6.25 ms
f715499 1234.26 ms 1259.40 ms 25.14 ms

App size

Revision Plain With Sentry Diff
7fb7afb 20.76 KiB 419.69 KiB 398.94 KiB
a9103fe 20.76 KiB 426.95 KiB 406.19 KiB
725565a 20.76 KiB 426.11 KiB 405.35 KiB
2b19b82 21.58 KiB 542.18 KiB 520.59 KiB
965db8a 22.84 KiB 403.24 KiB 380.39 KiB
fc163f5 20.76 KiB 436.30 KiB 415.54 KiB
b16d18c 20.76 KiB 437.12 KiB 416.36 KiB
1734d1b 21.58 KiB 418.82 KiB 397.24 KiB
addcf42 22.85 KiB 413.42 KiB 390.57 KiB
f715499 20.76 KiB 427.23 KiB 406.47 KiB

Previous results on branch: fix/flush-directly-after-capture

Startup times

Revision Plain With Sentry Diff
840f529 1219.44 ms 1240.42 ms 20.98 ms
8621dd7 1331.67 ms 1377.76 ms 46.09 ms
7c38a5b 1237.53 ms 1261.33 ms 23.80 ms
7d94f14 1230.35 ms 1252.16 ms 21.81 ms

App size

Revision Plain With Sentry Diff
840f529 21.58 KiB 616.82 KiB 595.24 KiB
8621dd7 21.58 KiB 615.95 KiB 594.37 KiB
7c38a5b 21.58 KiB 616.87 KiB 595.29 KiB
7d94f14 21.58 KiB 616.76 KiB 595.18 KiB

@philipphofmann philipphofmann merged commit 379ff2a into main May 10, 2024
69 of 70 checks passed
@philipphofmann philipphofmann deleted the fix/flush-directly-after-capture branch May 10, 2024 07:56
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.

None yet

3 participants