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

Use the Tracks Sentry integration #12001

Merged
merged 5 commits into from
Jul 4, 2019

Conversation

jkmassel
Copy link
Contributor

@jkmassel jkmassel commented Jun 25, 2019

Integrate Sentry using Tracks

To Test:

  • Add these lines to a file and run the app in a simulator:
import AutomatticTracks

CrashLogging.logMessage("This is a test!")

Run the app in a way that causes the code above to run, and verify that it shows up in Sentry.

@jkmassel jkmassel added the Tooling Build, Release, and Validation Tools label Jun 25, 2019
@jkmassel jkmassel added this to the 12.8 milestone Jun 25, 2019
@jkmassel jkmassel self-assigned this Jun 25, 2019
@jkmassel jkmassel force-pushed the use/tracks-version-of-sentry-library branch from 0a165ba to d2d6f47 Compare June 25, 2019 21:59
@jtreanor
Copy link
Contributor

There seems to be some failing tests here. I re-ran but it still seems to be red. develop is fine.

@jkmassel jkmassel force-pushed the use/tracks-version-of-sentry-library branch from d2d6f47 to c62d44d Compare June 26, 2019 16:23
@jkmassel
Copy link
Contributor Author

jkmassel commented Jul 1, 2019

Well this was a little more complicated than I first expected.

We use https://github.com/wordpress-mobile/NSObject-SafeExpectations which adds the objectWithKeyPath method to NSDictionary. As it turns out, Sentry also adds an objectWithKeyPath method. So when it's bundled statically, the method signatures conflict.

I've filed an issue with Sentry (getsentry/sentry-cocoa#314), because I don't think they're deliberately exporting these symbols for use outside the library.

In the mean time, I've disabled static compilation for Sentry and Tracks for the moment, but if we don't think that's an acceptable trade-off, we could see where Sentry lands with this and proceed at that time? We have an implementation that works, it's just inconsistent with the others. That bit of tech debt might be worth it for the optimizations provided by static compilation.

WDYT?

@jtreanor
Copy link
Contributor

jtreanor commented Jul 2, 2019

Ah, that is unexpected. This is a perfect example of why categories in libraries should always be prefixed 😄

I've disabled static compilation for Sentry and Tracks for the moment

I don't love this but I accept this may be the best trade off right now. Why does it need to be disabled for Tracks as well as Sentry? Perhaps I'm missing something.

Copy link
Contributor

@jtreanor jtreanor left a comment

Choose a reason for hiding this comment

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

Works as described. 🚢 it!

Clean up an extra comment

Fix code organization
Without this assertion, a test can crash two lines later on `componentsWithURL`. When the crash happens, the debugger can’t trace back to where it happened. Adding this assertion fixes that.
When Sentry is statically linked, it can overwrite symbols defined in NSObject+SafeExpectations. Until we can remove this Objective-C extension, we’ll need to special-case this.
@jkmassel jkmassel force-pushed the use/tracks-version-of-sentry-library branch from 9900835 to 5c00167 Compare July 4, 2019 20:38
@jkmassel jkmassel merged commit 7a00a14 into develop Jul 4, 2019
@jkmassel jkmassel deleted the use/tracks-version-of-sentry-library branch July 4, 2019 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tooling Build, Release, and Validation Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants