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

feat: Report usage of stitchAsyncCode #2281

Merged
merged 4 commits into from
Oct 19, 2022

Conversation

kevinrenskers
Copy link
Contributor

📜 Description

We can now report usage of the stitchAsyncCode option, as an integration.

💡 Motivation and Context

Closes #1944.

💚 How did you test it?

Unit test.

📝 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

@github-actions
Copy link

github-actions bot commented Oct 13, 2022

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

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

- Report usage of stitchAsyncCode ([#2281](https://github.com/getsentry/sentry-cocoa/pull/2281))

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 d9d2ca7

@kevinrenskers
Copy link
Contributor Author

One question about the SentryClient.setSdk method:

- (void)setSdk:(SentryEvent *)event
{
    if (event.sdk) {
        return;
    }

    id integrations = event.extra[@"__sentry_sdk_integrations"];
    if (!integrations) {
        integrations = [NSMutableArray new];
        for (NSString *integration in SentrySDK.currentHub.installedIntegrationNames) {
            // Every integration starts with "Sentry" and ends with "Integration". To keep the
            // payload of the event small we remove both.
            NSString *withoutSentry = [integration stringByReplacingOccurrencesOfString:@"Sentry"
                                                                             withString:@""];
            NSString *trimmed = [withoutSentry stringByReplacingOccurrencesOfString:@"Integration"
                                                                         withString:@""];
            [integrations addObject:trimmed];
        }

        if (self.options.stitchAsyncCode) {
            [integrations addObject:@"StitchAsyncCode"];
        }
    }

    event.sdk = @{
        @"name" : SentryMeta.sdkName,
        @"version" : SentryMeta.versionString,
        @"integrations" : integrations
    };
}

This line id integrations = event.extra[@"__sentry_sdk_integrations"];.. I can't find the string __sentry_sdk_integrations anywhere in the project. Is this for hybrid SDKs so they can set their own "secret" integrations string on an event? Or is this old code that should be removed?

@github-actions
Copy link

github-actions bot commented Oct 13, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1246.78 ms 1264.26 ms 17.48 ms
Size 20.50 KiB 337.84 KiB 317.34 KiB

Baseline results on branch: master

Startup times

Revision Plain With Sentry Diff
b869536 1250.37 ms 1274.84 ms 24.47 ms
791123d 1217.52 ms 1253.08 ms 35.56 ms
be47c6c 1230.39 ms 1261.71 ms 31.33 ms
654f180 1220.08 ms 1248.76 ms 28.67 ms
4a188b8 1229.81 ms 1255.96 ms 26.15 ms
7138b7d 1243.40 ms 1252.08 ms 8.68 ms
c61d869 1255.92 ms 1267.47 ms 11.55 ms
5025d2e 1248.52 ms 1251.72 ms 3.20 ms
2ce5819 1258.02 ms 1271.94 ms 13.92 ms
6177f2d 1206.55 ms 1226.20 ms 19.65 ms

App size

Revision Plain With Sentry Diff
b869536 20.51 KiB 331.79 KiB 311.28 KiB
791123d 20.51 KiB 331.81 KiB 311.30 KiB
be47c6c 20.50 KiB 333.54 KiB 313.04 KiB
654f180 20.50 KiB 335.95 KiB 315.45 KiB
4a188b8 20.50 KiB 337.70 KiB 317.20 KiB
7138b7d 20.51 KiB 331.79 KiB 311.28 KiB
c61d869 20.51 KiB 333.10 KiB 312.59 KiB
5025d2e 20.51 KiB 331.79 KiB 311.28 KiB
2ce5819 20.50 KiB 337.76 KiB 317.26 KiB
6177f2d 20.51 KiB 332.90 KiB 312.40 KiB

Previous results on branch: feature/1944-track-stitchAsyncCode

Startup times

Revision Plain With Sentry Diff
786aeb8 1232.65 ms 1252.02 ms 19.37 ms

App size

Revision Plain With Sentry Diff
786aeb8 20.50 KiB 333.57 KiB 313.07 KiB

@@ -1,5 +1,11 @@
# Changelog

## Unreleased
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to these changes, but I find it annoying to have to keep re-adding this after each release. I've filed an issue here proposing improvements, please check it out and +1 it if you agree! getsentry/craft#422

@armcknight
Copy link
Member

One question about the SentryClient.setSdk method:
...
This line id integrations = event.extra[@"__sentry_sdk_integrations"];.. I can't find the string __sentry_sdk_integrations anywhere in the project. Is this for hybrid SDKs so they can set their own "secret" integrations string on an event? Or is this old code that should be removed?

It appears in the Unity repo, in an old submodule version of this repo, I guess we removed it from here but Unity is using an old version (7.13.0, we're now on 7.28.0):

if (event.extra[@"__sentry_sdk_integrations"]) {
[sdk setValue:event.extra[@"__sentry_sdk_integrations"] forKey:@"integrations"];

Not sure if it's even used there ¯\(ツ)/¯ My grep would've also turned up hits in our docs, but it's not there either.

@kevinrenskers
Copy link
Contributor Author

Why is this PR not getting any reviews?

@kevinrenskers kevinrenskers merged commit 83f4441 into master Oct 19, 2022
@kevinrenskers kevinrenskers deleted the feature/1944-track-stitchAsyncCode branch October 19, 2022 08:50
@armcknight
Copy link
Member

@kevinrenskers I see you were asking for some reviews on this and #2263. Personally, I don't review PRs that have failing tests, to avoid review churn. Why were these PRs merged with the VLC and Home Assistant integration tests in a failure state?

@kevinrenskers
Copy link
Contributor Author

Both PRs had green checks actually.

After they were approved I had to fix the changelog conflict, and immediately merged without waiting for tests. After all, I only changed the changelog.

But yea they were both waiting for review with green checks.

@armcknight
Copy link
Member

Ah yeah I get you, that's annoying. There's a lot to say there about github's triggers, I've previously filed a support ticket for that and they said it works as intended 🙄

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.

Track how many people use stitchAsyncCode
3 participants