Automattic / Automattic-Tracks-iOS

Client library for tracking user events for later analysis
GNU General Public License v2.0
42 stars 12 forks source link

Blocker for Sentry version 7.x migration #181

Closed mokagio closed 2 years ago

mokagio commented 3 years ago

While looking at crash reports as part of the release management duties, I saw a notice from Sentry encouraging to upgrade to their latest version, 7.1.3.

I opened a WIP PR for it, but, unfortunately, there's a breaking change between 6.x and 7.x: SentrySDK doesn't expose the currentHub method, which we use here and here as part of our custom flow to manually log errors with attached stack trace and running a completion block (see https://github.com/getsentry/sentry-cocoa/issues/660).

Before opening an issue on the Sentry repo, I just wanted to double check here whether I may be missing something.

My current undersanding

We use that currentHub method to get the current SentryHub. The docs for this type are sparse:

You can think of the hub as the central point that our SDKs use to route an event to Sentry. When you call init() a hub is created and a client and a blank scope are created on it. That hub is then associated with the current thread and will internally hold a stack of scopes.

The scope will hold useful information that should be sent along with the event.

The docs also imply the average users is not supposed to interact with hubs, hence why they made the getter private, I'm guessing.

The hub you are unlikely to be interacting with directly unless you are writing an integration or you want to create or destroy scopes.

We use the hub only to get references to the current client and scope. We need an instance of the client because we call a method on it. I'm also pretty sure we need an instance of the scope. "The scope will hold useful information," if we were to init that ourselves because we can't get one from the hub, we'd lose all the current information.

The way forward

I looped through the autocompletion on SentrySDK and SentryHub looking for a promising method to supplement what we can't do anymore, but came up short.

I also run the demo app, hacking Tracks so that it would compile, to verify that Client responds to the prepareEvent:withScope:alwaysAttachStacktrace:isCrashEvent: selector which is the one we built our shim around. If it didn't, we would have definitely have to go back to the drawing board, but luckily (?) it does.

So, with the info I have at this point, the only way forward I see is opening an issue on the Sentry SDK repo, asking for help, ideally for reintroducing the currentHub method.

Here's a draft:

Question

Since version 7.x removed the currentHub and setCurrentHub from SentrySDK, what's the way to get a reference to the current hub now?

Context

With version 6, we use the hub to get a reference to the current client and current scope in order to attach stack traces to events we log manually.

This relates to https://github.com/getsentry/sentry-cocoa/issues/660.

Suggestion

Would it perhaps be possible to reintroduce the currentHub getter on SentrySDK? Being only a getter method, it should be "harmless" for users to grab a reference and inspect its properties, right? I can see why you wouldn't want users to override the hub, as your docs say "The hub you are unlikely to be interacting with directly unless you are writing an integration or you want to create or destroy scopes," but, for us at least, having the ability to read what's in the hub is crucial.

Thank you for your help


What do you think, am I missing something? If not, does my question and suggestion for the Sentry team make sense? Is there anything that I could add to make it easier to understand?

Also, feel free to take a stab at the code in #180 to see if you can get it to work πŸ˜„

Thanks πŸ™‡β€β™‚οΈ

spencertransier commented 2 years ago

@mokagio Is this still a blocker for upgrading Sentry to 7.x? The Day One team was wondering if upgrading to the latest Sentry SDK is now possible.

mokagio commented 2 years ago

Hey @spencertransier πŸ‘‹ Yeah, this is still a blocker. I haven't had time to look into how to rewrite or update our implementation to work around the APIs that were removed when going from 6.x to 7.x 😞

Maybe you or someone working on Day One can?

Here's two issues on the Sentry repo where we discuss about some options:

Maybe @jkmassel has some helpful ideas on where to start.