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: Don't start the SDK inside Xcode preview #4601

Open
wants to merge 20 commits into
base: feat/swiftui-ttid
Choose a base branch
from

Conversation

brustolin
Copy link
Contributor

📜 Description

The SDK was running inside Xcode preview for SwiftUI and slowing things down for development.

💚 How did you test it?

Sample and Unit test

📝 Checklist

You have to check all boxes before merging:

  • 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.
  • I updated the wizard 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

github-actions bot commented Dec 5, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1222.94 ms 1241.67 ms 18.73 ms
Size 22.31 KiB 756.92 KiB 734.61 KiB

Baseline results on branch: feat/swiftui-ttid

Startup times

Revision Plain With Sentry Diff
8b6def0 1226.11 ms 1247.20 ms 21.10 ms
51e85b4 1228.22 ms 1249.19 ms 20.96 ms
9ef5a81 1234.29 ms 1256.35 ms 22.07 ms
895b829 1232.55 ms 1250.90 ms 18.35 ms
f6a9e2b 1234.94 ms 1245.60 ms 10.66 ms
8c646bc 1221.49 ms 1245.31 ms 23.82 ms
f583fd1 1243.27 ms 1258.91 ms 15.65 ms
f38e16a 1237.13 ms 1262.76 ms 25.62 ms
7ce0058 1230.49 ms 1246.94 ms 16.45 ms

App size

Revision Plain With Sentry Diff
8b6def0 22.30 KiB 750.46 KiB 728.16 KiB
51e85b4 22.31 KiB 756.73 KiB 734.42 KiB
9ef5a81 22.30 KiB 750.46 KiB 728.16 KiB
895b829 22.30 KiB 750.46 KiB 728.16 KiB
f6a9e2b 22.30 KiB 750.46 KiB 728.16 KiB
8c646bc 22.31 KiB 756.74 KiB 734.44 KiB
f583fd1 22.30 KiB 750.46 KiB 728.16 KiB
f38e16a 22.30 KiB 750.46 KiB 728.16 KiB
7ce0058 22.30 KiB 750.46 KiB 728.16 KiB

Previous results on branch: feat/disableSDKSwiftUIPreview

Startup times

Revision Plain With Sentry Diff
b421d36 1234.29 ms 1249.18 ms 14.89 ms
db8a1d5 1245.84 ms 1263.08 ms 17.24 ms
3965049 1228.13 ms 1242.18 ms 14.06 ms
0cb9336 1223.10 ms 1244.09 ms 20.99 ms
9ce8938 1233.04 ms 1254.67 ms 21.63 ms
c994cb9 1231.63 ms 1255.37 ms 23.74 ms
3578731 1221.36 ms 1245.63 ms 24.28 ms

App size

Revision Plain With Sentry Diff
b421d36 22.30 KiB 750.94 KiB 728.64 KiB
db8a1d5 22.30 KiB 750.94 KiB 728.64 KiB
3965049 22.30 KiB 750.83 KiB 728.52 KiB
0cb9336 22.30 KiB 750.89 KiB 728.59 KiB
9ce8938 22.30 KiB 750.82 KiB 728.52 KiB
c994cb9 22.30 KiB 750.89 KiB 728.59 KiB
3578731 22.30 KiB 750.89 KiB 728.59 KiB

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/Sentry/SentrySDK.m Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryTracedView.swift Outdated Show resolved Hide resolved
Sources/Sentry/SentrySDK.m Outdated Show resolved Hide resolved
Tests/SentryTests/SentrySDKTests.swift Outdated Show resolved Hide resolved
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.

I'm not sure about the whole approach of the PR. What exactly is causing the slow-down? Can we do something to improve this? Maybe we should only turn off certain parts of the SDK. It's not obvious that we do this, and some users might get confused.

@brustolin
Copy link
Contributor Author

I'm not sure about the whole approach of the PR. What exactly is causing the slow-down? Can we do something to improve this? Maybe we should only turn off certain parts of the SDK. It's not obvious that we do this, and some users might get confused.

Do you have any reason why should Sentry run inside Xcode preview?

@philipphofmann
Copy link
Member

Do you have any reason why should Sentry run inside Xcode preview?

Some users might expect it to and have reasons and get confused that we deactivate it. I haven't seen this approach in any other SDK. Again, I would like to understand what's causing the slowdown before deactivating it.

@brustolin
Copy link
Contributor Author

brustolin commented Dec 6, 2024

Some users might expect it to and have reasons and get confused that we deactivate it.

I disagree; No user expects their monitoring solution to be running inside of Xcode preview, that is meant to help with creation of screens. There is no relevant information that could be used out of it, and the SDK would be initialized and dealloc many times in sequence, we don’t even know what the expected behavior of such usage is.

Again, I would like to understand what's causing the slowdown before deactivating it.

The entire SDK. In order to see my SwiftUI preview the SDK needs to initialize all its integrations, needs to create a new thread to monitor Apphangs, needs to swizzle a bunch of functions and make an analysis of the startup time, and none of this will be used.

Again, what's the use case to run the SDK inside Xcode preview?

Sources/Sentry/SentrySDK.m Outdated Show resolved Hide resolved
Sources/Sentry/SentrySDK.m Outdated Show resolved Hide resolved
@philprime
Copy link
Contributor

I agree with @brustolin on this one.

Xcode preview supports a static Preview Mode and an interactive Live Mode (Docs). As the preview can be used for individual UI components, it is most likely used in some kind of container framework/application (which then runs on the simulator in live mode), therefore not the full application as if it would run normally.

Letting users think they can use the SDK in this environment, might actually cause confusion down the road.

Regarding the performance, not sure how heavy the impact is, but generally the body method of any view should be as quickly as possible, especially when redraw happens many times like in preview.

@philipphofmann
Copy link
Member

It seems wrong that we have to add this logic to our SDK. Why is the SDK even running in the Preview? Isn't this something our users should ensure doesn't happen? The preview shouldn't start Sentry. That should be in the @main init of the App. It seems to me we're fixing problems that our users should take care of. Again, I didn't see this in any other SDK yet.

@armcknight
Copy link
Member

armcknight commented Dec 6, 2024

In my testing, @main.init() always runs as part of executing a preview. So this is not a customer error IMO. I did find one SDK that throws an error, indicating there must be something the customer could do to avoid it: https://github.com/embrace-io/embrace-apple-sdk/blob/485fe0b31470427cb86cae36b1a88302f7db9b4a/Sources/EmbraceCore/Embrace.swift#L101-L103, but ultimately I think it would wind up being the same thing we do in this PR, so we might as well do it instead of forcing thousands of duplicated implementations of the same thing.

I agree that most things in Sentry should not run in Xcode previews. (Maybe once we launch User Feedback, we may want that to be previewable, but that can also be something we handle when the time comes, after merging something like this). I found lots of other dev tools on GitHub that are looking at this environment variable to decide whether or not to do things:

I think we should ultimately adopt a similar pattern to CopilotForXcode, Dashlane or PointFreeCo/swift-dependencieas, so that integrations and other basic components (like launch profiling which runs independently of SentrySDK.start, or if we ever implement remote config) can be selectively disabled in previews, instead of the entire SDK.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

A question about using TestSentryNSProcessInfoWrapper, but otherwise I agree we should move forward with this.

Sources/Sentry/SentrySDK.m Outdated Show resolved Hide resolved
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.

You all convinced me. Thanks for the PR, @brustolin, and thanks for all the extra context, @armcknight. LGTM when adding some log messages.

Comment on lines +623 to +636
#if TEST || TESTCI
static NSDictionary<NSString *, NSString *> *_processInfoEnvironment;

+ (void)setProcessInfoEnvironment:(NSDictionary<NSString *, NSString *> *)dictionary
{
_processInfoEnvironment = dictionary;
}

+ (NSDictionary<NSString *, NSString *> *)processInfoEnvironment
{
return _processInfoEnvironment;
}

#endif
Copy link
Member

Choose a reason for hiding this comment

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

m: That code isn't used.

@@ -201,6 +203,11 @@ + (void)setStartTimestamp:(NSDate *)value

+ (void)startWithOptions:(SentryOptions *)options
{
if ([SentryDependencyContainer.sharedInstance.processInfoWrapper
.environment[SENTRY_XCODE_PREVIEW_ENVIRONMENT_KEY] isEqualToString:@"1"]) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

m: Please add a WARN log message to communicate why we don't start the SDK.

private func shouldTrace() -> Bool {
#if DEBUG
if ProcessInfo.processInfo.environment[SENTRY_XCODE_PREVIEW_ENVIRONMENT_KEY] == "1" {
return false
Copy link
Member

Choose a reason for hiding this comment

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

m: Please add a log message explaining why we don't trace.

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