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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Don't run onCrashedLastSession for nil Events #3785

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

brustolin
Copy link
Contributor

馃摐 Description

Don't run onCrashedLastSession for nil Events. This could happen if beforeSend returns nil

馃挕 Motivation and Context

closes #3784

馃挌 How did you test it?

Unit test

馃摑 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

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 89.345%. Comparing base (742d4b6) to head (092b365).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3785       +/-   ##
=============================================
- Coverage   89.366%   89.345%   -0.022%     
=============================================
  Files          548       548               
  Lines        59766     59786       +20     
  Branches     21474     21484       +10     
=============================================
+ Hits         53411     53416        +5     
- Misses        5315      5327       +12     
- Partials      1040      1043        +3     
Files Coverage 螖
Sources/Sentry/SentryClient.m 96.028% <100.000%> (+0.007%) 猬嗭笍
Tests/SentryTests/SentryClientTests.swift 95.915% <86.666%> (-0.093%) 猬囷笍

... and 14 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 742d4b6...092b365. Read the comment docs.

Copy link

github-actions bot commented Mar 25, 2024

Performance metrics 馃殌

Plain With Sentry Diff
Startup time 1237.76 ms 1256.94 ms 19.18 ms
Size 21.58 KiB 547.02 KiB 525.44 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
01a28a9 1225.55 ms 1249.96 ms 24.41 ms
de46f06 1255.53 ms 1268.44 ms 12.91 ms
3f366ee 1242.28 ms 1260.80 ms 18.52 ms
75ef4eb 1237.30 ms 1249.53 ms 12.23 ms
72c8d84 1238.96 ms 1247.34 ms 8.38 ms
105a36c 1244.76 ms 1249.12 ms 4.36 ms
b8dd0fc 1235.57 ms 1253.12 ms 17.55 ms
7cd187e 1223.41 ms 1249.40 ms 26.00 ms
3cb68af 1221.15 ms 1238.40 ms 17.25 ms
af1f4dd 1238.08 ms 1258.48 ms 20.40 ms

App size

Revision Plain With Sentry Diff
01a28a9 22.85 KiB 405.39 KiB 382.55 KiB
de46f06 22.85 KiB 414.74 KiB 391.89 KiB
3f366ee 20.76 KiB 427.84 KiB 407.08 KiB
75ef4eb 22.85 KiB 413.45 KiB 390.60 KiB
72c8d84 22.85 KiB 408.88 KiB 386.03 KiB
105a36c 22.85 KiB 414.09 KiB 391.24 KiB
b8dd0fc 20.76 KiB 401.39 KiB 380.63 KiB
7cd187e 20.76 KiB 401.66 KiB 380.89 KiB
3cb68af 20.76 KiB 401.60 KiB 380.84 KiB
af1f4dd 22.85 KiB 414.71 KiB 391.86 KiB

Previous results on branch: fix/fix-onCrashLastRun-for-null-events

Startup times

Revision Plain With Sentry Diff
e9fa12b 1214.63 ms 1234.06 ms 19.43 ms

App size

Revision Plain With Sentry Diff
e9fa12b 21.58 KiB 546.23 KiB 524.65 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

That makes sense. Thanks @brustolin.

@brustolin brustolin merged commit db533ee into main Mar 26, 2024
44 of 45 checks passed
@brustolin brustolin deleted the fix/fix-onCrashLastRun-for-null-events branch March 26, 2024 07:45
Copy link

Fails
馃毇 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Don't run onCrashedLastSession for nil Events ([#3785](https://github.com/getsentry/sentry-cocoa/pull/3785))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 馃毇 dangerJS against 2e08332

threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
Don't run onCrashedLastSession for nil Events. This could happen if beforeSend returns nil
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.

onCrashedLastRun block is called even if beforeSend sets the event to nil
2 participants