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

Expose Sentry._Hybrid explicit module #4440

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Oct 15, 2024

📜 Description

We want to support SPM in our sentry flutter framework. In order to do this, we need the hybrid classes, which we expose though Cocoapods, to also be available in our SPM package.

This PR adds an explicit module Sentry._Hybrid with the hybrid headers.

💡 Motivation and Context

Unblocks getsentry/sentry-dart#2280

#if SWIFT_PACKAGE
import Sentry._Hybrid
#endif

💚 How did you test it?

📝 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.

Copy link

github-actions bot commented Oct 15, 2024

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

Generated by 🚫 dangerJS against a4bca18

Copy link

github-actions bot commented Oct 15, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1229.77 ms 1255.90 ms 26.13 ms
Size 22.30 KiB 730.75 KiB 708.45 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
879fb28 1243.18 ms 1255.98 ms 12.80 ms
98752f3 1226.18 ms 1251.38 ms 25.20 ms
e0904ef 1231.85 ms 1252.38 ms 20.53 ms
3f6c30b 1252.98 ms 1266.22 ms 13.24 ms
26530fe 1233.98 ms 1250.06 ms 16.08 ms
742d4b6 1196.56 ms 1216.54 ms 19.98 ms
8b1c6a9 1216.92 ms 1230.90 ms 13.98 ms
643853e 1225.75 ms 1247.00 ms 21.25 ms
8e76be4 1272.67 ms 1286.38 ms 13.71 ms
dacf894 1223.96 ms 1250.41 ms 26.45 ms

App size

Revision Plain With Sentry Diff
879fb28 22.84 KiB 402.88 KiB 380.03 KiB
98752f3 20.76 KiB 435.09 KiB 414.33 KiB
e0904ef 21.58 KiB 614.64 KiB 593.06 KiB
3f6c30b 22.85 KiB 408.88 KiB 386.03 KiB
26530fe 21.58 KiB 714.93 KiB 693.35 KiB
742d4b6 21.58 KiB 546.20 KiB 524.62 KiB
8b1c6a9 21.58 KiB 706.97 KiB 685.38 KiB
643853e 21.58 KiB 655.73 KiB 634.15 KiB
8e76be4 20.76 KiB 427.66 KiB 406.89 KiB
dacf894 20.76 KiB 426.34 KiB 405.58 KiB

Previous results on branch: feat/hybrid-sdk-private-modulemap

Startup times

Revision Plain With Sentry Diff
3dc0c00 1235.20 ms 1250.67 ms 15.47 ms
6b94106 1232.12 ms 1253.14 ms 21.02 ms
440380a 1233.94 ms 1251.60 ms 17.66 ms
5ddaad1 1234.75 ms 1252.96 ms 18.21 ms
25f6d46 1238.86 ms 1258.98 ms 20.12 ms

App size

Revision Plain With Sentry Diff
3dc0c00 21.90 KiB 726.90 KiB 705.00 KiB
6b94106 21.90 KiB 726.89 KiB 704.99 KiB
440380a 21.90 KiB 708.33 KiB 686.43 KiB
5ddaad1 21.90 KiB 730.10 KiB 708.20 KiB
25f6d46 22.30 KiB 730.10 KiB 707.80 KiB

@denrase
Copy link
Collaborator Author

denrase commented Oct 15, 2024

@philipphofmann Maybe it's enough to mark the headers in question as package private, wdyt?

@denrase denrase marked this pull request as ready for review October 15, 2024 11:24
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, it looks good.

Sources/Resources/Sentry.modulemap Outdated Show resolved Hide resolved
Sentry.xcodeproj/project.pbxproj Show resolved Hide resolved
@brustolin
Copy link
Contributor

We will need a test workflow of sample using PrivateSentrySDKOnly from Swift, so we make sure to not break it.

@denrase
Copy link
Collaborator Author

denrase commented Oct 29, 2024

Closed in favor of #4486

@denrase denrase closed this Oct 29, 2024
@denrase denrase reopened this Nov 6, 2024
@denrase
Copy link
Collaborator Author

denrase commented Nov 6, 2024

@brustolin @philipphofmann Turns our we do need the explicit module after all to import the needed files in Flutter when integrating through SPM. Therefore i reopened this PR.

@denrase denrase mentioned this pull request Nov 6, 2024
10 tasks
@denrase denrase changed the title Expose HybridSDK from Sentry framework Expose Sentry._Hybrid from Sentry framework Nov 6, 2024
@denrase denrase changed the title Expose Sentry._Hybrid from Sentry framework Expose Sentry._Hybrid explicit module Nov 6, 2024
@denrase denrase marked this pull request as draft November 6, 2024 14:33
@denrase denrase marked this pull request as ready for review November 6, 2024 15:43
@denrase denrase marked this pull request as draft November 6, 2024 15:44
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.575%. Comparing base (d9f518a) to head (a4bca18).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4440       +/-   ##
=============================================
+ Coverage   91.566%   91.575%   +0.008%     
=============================================
  Files          615       615               
  Lines        69701     69689       -12     
  Branches     24967     24957       -10     
=============================================
- Hits         63823     63818        -5     
+ Misses        5787      5779        -8     
- Partials        91        92        +1     
Files with missing lines Coverage Δ
...tions/SessionReplay/SentrySessionReplayTests.swift 98.870% <100.000%> (ø)

... and 9 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 d9f518a...a4bca18. Read the comment docs.

@denrase denrase marked this pull request as ready for review November 11, 2024 14:47
@denrase denrase marked this pull request as draft November 11, 2024 15:15
@denrase denrase marked this pull request as ready for review November 11, 2024 15:19
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.

We need to implement some CI tests to prevent regression, as it’s easy to break since this is only used by hybrid SDKs.

Maybe trying to access Sentry._Hybrid from the test would be enough.

Comment on lines +9 to +30
header "PrivateSentrySDKOnly.h"
header "PrivatesHeader.h"
header "SentryAppStartMeasurement.h"
header "SentryBinaryImageCache.h"
header "SentryBreadcrumb+Private.h"
header "SentryDebugImageProvider+HybridSDKs.h"
header "SentryDependencyContainer.h"
header "SentryEnvelope.h"
header "SentryEnvelopeItemType.h"
header "SentryFormatter.h"
header "SentryFramesTracker.h"
header "SentryOptions+HybridSDKs.h"
header "SentryScreenFrames.h"
header "SentrySwizzle.h"
header "SentryUser+Private.h"

header "SentryBaseIntegration.h"
header "SentrySessionReplayIntegration.h"
header "SentrySessionReplayIntegration-Hybrid.h"

header "SentrySdkInfo.h"
header "SentryInternalSerializable.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

l: Is it possible to have one header that has all this other headers as reference? The same way we do with Sentry.h.

Just because a header can be more versatile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had some issues with this approach, but will try again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I always get an error about non-modular headers when i try to move the header imports into one header.

Bildschirmfoto 2024-11-13 um 14 10 00

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, no problem. We can continue without it.

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.

4 participants