Skip to content

Conversation

@vaind
Copy link
Collaborator

@vaind vaind commented Jun 19, 2024

📜 Description

This exposes a define SENTRY_TARGET_REPLAY_SUPPORTED (there's already SENTRY_TARGET_PROFILING_SUPPORTED so keeping the name format) for hybrid SDKs so they don't have to copy the specific check SENTRY_HAS_UIKIT && !TARGET_OS_VISION which may change in the future, e.g. when replay for macOS is made available. Also, while adding this, I've noticed that some replay functions were exported & then logged a warning if replay wasn't available, while some were not exported at all (see link below for the RN PR). I've unified this so that no functions are exported for hybrid SDKs if replay is not available.

💡 Motivation and Context

Came up in a compilation fail for RN getsentry/sentry-react-native#3846 (comment)

💚 How did you test it?

All the original code & tests should pass. Except for tests that tried if the interfaces didn't throw when replay isn't available, obviously, since those are removed now.

📝 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

Use this new define in RN and Flutter.

#skip-changelog because this change isn't for normal SDK consumers.

@codecov
Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.113%. Comparing base (f1c36e0) to head (5a0f986).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4089       +/-   ##
=============================================
+ Coverage   91.065%   91.113%   +0.047%     
=============================================
  Files          610       610               
  Lines        47750     47812       +62     
=============================================
+ Hits         43484     43563       +79     
+ Misses        4266      4249       -17     
Files Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.mm 90.780% <ø> (+0.435%) ⬆️
Sources/Sentry/SentryCrashWrapper.m 94.520% <100.000%> (ø)
Sources/Sentry/SentrySessionReplay.m 86.111% <ø> (ø)
Sources/Sentry/SentrySessionReplayIntegration.m 90.322% <ø> (ø)
Tests/SentryTests/PrivateSentrySDKOnlyTests.swift 99.551% <ø> (-0.012%) ⬇️

... and 16 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 f1c36e0...5a0f986. Read the comment docs.

@vaind vaind changed the title chore: add SENTRY_REPLAY_AVAILABLE define chore: cleanup replay exports for hybrid SDKs Jun 19, 2024
@vaind vaind marked this pull request as ready for review June 19, 2024 10:07
@vaind vaind requested review from brustolin and krystofwoldrich and removed request for armcknight, brustolin and philipphofmann June 19, 2024 10:07
@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1221.69 ms 1229.78 ms 8.08 ms
Size 21.58 KiB 670.40 KiB 648.82 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3b782cc 1261.67 ms 1272.24 ms 10.57 ms
2172278 1229.58 ms 1245.00 ms 15.42 ms
ed68562 1238.45 ms 1251.57 ms 13.12 ms
965db8a 1211.61 ms 1226.60 ms 14.99 ms
06548c0 1225.58 ms 1244.70 ms 19.12 ms
c0c1496 1201.19 ms 1228.36 ms 27.17 ms
f938d24 1223.26 ms 1242.12 ms 18.86 ms
ecd9ecd 1241.28 ms 1260.35 ms 19.07 ms
42ef6ba 1211.20 ms 1228.17 ms 16.96 ms
f0283e8 1245.92 ms 1262.82 ms 16.90 ms

App size

Revision Plain With Sentry Diff
3b782cc 20.76 KiB 432.21 KiB 411.45 KiB
2172278 21.58 KiB 542.28 KiB 520.70 KiB
ed68562 22.84 KiB 403.24 KiB 380.39 KiB
965db8a 22.84 KiB 403.24 KiB 380.39 KiB
06548c0 20.76 KiB 427.35 KiB 406.59 KiB
c0c1496 22.85 KiB 407.45 KiB 384.60 KiB
f938d24 20.76 KiB 434.88 KiB 414.12 KiB
ecd9ecd 20.76 KiB 420.23 KiB 399.47 KiB
42ef6ba 21.58 KiB 417.86 KiB 396.28 KiB
f0283e8 20.76 KiB 393.37 KiB 372.61 KiB

Previous results on branch: chore/add-sentry-replay-define

Startup times

Revision Plain With Sentry Diff
df4da79 1208.67 ms 1222.04 ms 13.37 ms

App size

Revision Plain With Sentry Diff
df4da79 21.58 KiB 670.40 KiB 648.82 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.

Thanks!! Easier to maintain this way.

Copy link
Contributor

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

Looks good, will be much easier in the future, but we should still log out when the replay is not available in RN code.

Thank you.

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.

Thank you @vaind 💯

@vaind vaind merged commit 1e1d6a5 into main Jun 20, 2024
@vaind vaind deleted the chore/add-sentry-replay-define branch June 20, 2024 09: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.

5 participants