Automattic / Automattic-Tracks-iOS

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

Updates to sentry 7.24.1 #224

Closed diegoreymendez closed 2 years ago

diegoreymendez commented 2 years ago

Updates to Sentry 7.24.1

Resolves https://github.com/Automattic/Automattic-Tracks-iOS/issues/181 (through a hack, though)

Details:

I posted a request in one of our old issues in the Sentry repo so that we can hopefully avoid having these hacks in-place.

Unfortunately I don't think it's possible to avoid these hacks right now, until we do larger changes on our end.

This PR aims specifically to unblock us and allow us to update to the latest Sentry version.

Testing:

You'll need to set-up the DSN in Secrets.swift.

  1. Place breakpoints in the places where I changed the code to see that the selectors don't fail and return reasonable values.
  2. Launch the Demo iOS app.
  3. Select "Crashes" in the bottom tab bar.
  4. Tap on "Send Error and Wait" and inspect the values in the breakpoints you placed to ensure we're getting the right data.
diegoreymendez commented 2 years ago

I'm a bit unsure why each step is red or why there are danger icons, but I don't think it's a major are of concern because, as observed elsewhere, we have a very limited use case for sending manual events and looking at their stack trace.

I've been seeing these yesterday (outside of my PR), and I think it has to do with Sentry not having the latest symbols for that binary.

Is this upgrade something you are on a rush to merge? I'm asking because, as you might have seen, I started https://github.com/Automattic/Automattic-Tracks-iOS/pull/220.

If you are not in a rush to merge, I would cherry-pick your changes into my branch, which I hope to get to a ready for review place next week. This is also a good time to talk about version numbers, and I https://github.com/Automattic/Automattic-Tracks-iOS/issues/225.

Is there an advantage in coupling both PRs vs merging this one first?

I'd strongly prefer this be pushed on its own unless there's a reason to want to hold it back.

It will be fairly easy to push iterative improvements on top of these even after they're merged.

diegoreymendez commented 2 years ago

Thanks @mokagio !

I'm happy to adjust the versioning though so let me know what'd be best!

diegoreymendez commented 2 years ago

@mokagio - I'm guessing you're referring to the fact we're doing a big change but not reflecting it through the versioning?

If that's the case I'd be fine with adjusting the versioning and changing the tags I'm using. I realize it's not ideal after it's published, but I'd be happy to help.

mokagio commented 2 years ago

@diegoreymendez the work I've been doing on Sentry v7 is focused at adding support for configuring performance tracking.

My initial implementation introduced a breaking change (but then @leandroalonso suggested a non-breaking alternative).

When I left the comment, I was still thinking in terms of shipping a breaking change and I would have liked to reflect it in the version number. Under the new circumstances I think:

Thank you for offering to help with updating the release version anyway. I really appreciate it.

diegoreymendez commented 2 years ago

Thanks @mokagio! That makes sense, and I agree that since most of what we're doing is "invisible" to the Apps it may be fine to go for minor version increases.