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

ref: SentrySDK.close calls flush #2453

Merged
merged 11 commits into from
Dec 1, 2022
Merged

ref: SentrySDK.close calls flush #2453

merged 11 commits into from
Dec 1, 2022

Conversation

philipphofmann
Copy link
Member

📜 Description

SentrySDK.close now calls flush and disables the client.

💡 Motivation and Context

Close should call flush.

💚 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
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

SentrySDK.close now calls flush and disables the client.
@github-actions
Copy link

github-actions bot commented Nov 28, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.02 ms 1258.58 ms 28.56 ms
Size 20.75 KiB 383.87 KiB 363.12 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
ae5ecd1 1226.73 ms 1250.96 ms 24.23 ms
034ff5e 1231.22 ms 1255.37 ms 24.14 ms
1ef804d 1243.26 ms 1255.90 ms 12.64 ms
68094b3 1214.14 ms 1255.09 ms 40.95 ms
1ce879f 1258.12 ms 1260.90 ms 2.78 ms
e2cec76 1189.48 ms 1229.84 ms 40.36 ms
7eee302 1228.73 ms 1241.94 ms 13.21 ms
cdf9acd 1219.62 ms 1254.80 ms 35.18 ms
a9db8a6 1199.83 ms 1234.56 ms 34.73 ms
fb9f27a 1218.55 ms 1249.17 ms 30.62 ms

App size

Revision Plain With Sentry Diff
ae5ecd1 20.75 KiB 383.31 KiB 362.55 KiB
034ff5e 20.75 KiB 383.83 KiB 363.07 KiB
1ef804d 20.75 KiB 383.40 KiB 362.65 KiB
68094b3 20.75 KiB 373.94 KiB 353.19 KiB
1ce879f 20.75 KiB 381.81 KiB 361.06 KiB
e2cec76 20.75 KiB 381.81 KiB 361.06 KiB
7eee302 20.75 KiB 374.73 KiB 353.97 KiB
cdf9acd 20.75 KiB 383.78 KiB 363.03 KiB
a9db8a6 20.75 KiB 383.37 KiB 362.62 KiB
fb9f27a 20.75 KiB 382.11 KiB 361.36 KiB

Previous results on branch: ref/sdk-close

Startup times

Revision Plain With Sentry Diff
68d487c 1265.57 ms 1276.22 ms 10.65 ms
00c0e6c 1197.57 ms 1232.56 ms 34.99 ms
ce157e9 1247.04 ms 1266.16 ms 19.12 ms
be7358c 1239.43 ms 1254.64 ms 15.21 ms
a1879cb 1221.71 ms 1248.00 ms 26.29 ms
9418710 1260.10 ms 1269.20 ms 9.10 ms
c9fbf80 1211.67 ms 1238.06 ms 26.39 ms
012d5ad 1219.22 ms 1239.43 ms 20.21 ms

App size

Revision Plain With Sentry Diff
68d487c 20.75 KiB 383.65 KiB 362.89 KiB
00c0e6c 20.75 KiB 383.51 KiB 362.76 KiB
ce157e9 20.75 KiB 383.87 KiB 363.12 KiB
be7358c 20.75 KiB 383.87 KiB 363.11 KiB
a1879cb 20.75 KiB 383.87 KiB 363.12 KiB
9418710 20.75 KiB 383.87 KiB 363.12 KiB
c9fbf80 20.75 KiB 383.64 KiB 362.88 KiB
012d5ad 20.75 KiB 383.50 KiB 362.75 KiB

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.

LGTM

Co-authored-by: Dhiogo Brustolin <[email protected]>
Co-authored-by: Dhiogo Brustolin <[email protected]>
CHANGELOG.md Outdated
@@ -42,6 +42,7 @@ This version adds a dependency on Swift.
- Rename `SentryOptions.enablePreWarmedAppStartTracking` to `enablePreWarmedAppStartTracing`
- Rename `SentryOptions.enableFileIOTracking` to `enableFileIOTracing`
- Rename `SentryOptions.enableCoreDataTracking` to `enableCoreDataTracing`
- SentrySDK.close calls flush (#2453)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really a breaking change? Or should this just be moved to the fixes section?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's now a blocking call. Previously, it wasn't. Would you consider that as a breaking change or not, @kevinrenskers?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so, plus it doesn't actually say that, in case you do think that makes it a breaking change. I think we'd better move it or explain how it's breaking?

Your call though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added that it is blocking. Imagine you call this somewhere on the main thread and have plenty of envelopes in the queue. Previously, this call was fast, and now it blocks your main thread for 1 second. I rather point it out as a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes perfect sense!

@philipphofmann philipphofmann merged commit 8e06104 into 8.0.0 Dec 1, 2022
@philipphofmann philipphofmann deleted the ref/sdk-close branch December 1, 2022 07: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.

3 participants