Update Sentry to v6 by jkmassel · Pull Request #166 · Automattic/Automattic-Tracks-iOS

jkmassel · 2021-01-16T06:26:45Z

Updates the Sentry library to v6, which enables:

Session tracking correlated with crashes to give us better crash rate data
Support for building the project on arm64 macs

Unfortunately, the v6 library takes away our ability to manually send events to Sentry, so this PR adds that ability back by using the API directly.

Previously to this PR, testing Sentry integration was always a bit tricky because we'd need to integrate into a host app and make weird changes to it in order to test the integration.

This PR adds the ability to test Sentry functionality from the example project.

To Test:

cd into the TracksDemo directory and run bundle exec pod install
Create the Shared/Secrets.swift file:
- For automatticians: copy the file from our mobile secrets repo (under iOS/tracks-demo). You might need to update (git pull) the mobile secrets repo first.
- For non-automatticians: copy the file Shared/Secrets.example.swift and fill in the missing details
- Feel free to customize your email address :)
- Note: In a future PR, we'll integrate the configure functionality to automate this, but it's not part of this PR in order to avoid making it any larger.
Run the sample project on iOS. Go to the "Crashes" tab and try "Send Test Event" and "Send Error and Wait". Ensure that both events show up in the Sentry tracks-demo project with an associated stack trace.
Run the same test on the macOS project.

Additional M1-specific tests:

Try building for Any iOS Device
Try building for My Mac, and Any Mac (Apple Silicon, Intel) [ note: you may need to clean your build directory first – this tends to fail otherwise – I assume this is an Xcode bug ]

I'd love one review on this from a mobile infrastructure engineer as well as a review from someone who has M1 hardware to test with.

jkmassel · 2021-01-18T22:00:44Z

Automattic-Tracks-iOS/Crash Logging/CrashLogging.swift

 import Sentry
 import CocoaLumberjack

+struct CrashLoggingInternals {


This class isn't fully untangled from all the static business, so we keep an internal-only static reference around until we can get rid of it.

I like the idea of shoving all of the stuff that we cannot easily migrate into a dedicated place to make it easier to find and change or remove later 👍

I'm addicted to nested types recently. I can see this fit nicely as one

public class CrashLogging { /// Comment explaining why this is here... class Internals { // ... } // ... Internals.crashLogging = self // ... }

No need to implement it if you don't see the benefit. Just playing code golf.

I like this idea a lot – this is done in 389e8c1

jkmassel · 2021-01-18T22:01:47Z

Automattic-Tracks-iOS/Crash Logging/CrashLogging.swift

 /// A class that provides support for logging crashes. Not compatible with Objective-C.
 public class CrashLogging {

-    /// A singleton is maintained, but the host application needn't be aware of its existence.


No more static instance internally – makes things safer, easier to test, and lets us remove a bunch of code.

jkmassel · 2021-01-18T22:02:24Z

Automattic-Tracks-iOS/Crash Logging/CrashLogging.swift

-     - SeeAlso: CrashLoggingDataProvider
-     */
-    public static func start(withDataProvider dataProvider: CrashLoggingDataProvider, eventLogging: EventLogging? = nil) {
+    public func start() throws -> CrashLogging {


throws because the DSN may be invalid, and we don't want to hide that error – otherwise we might ship an app to production with no ability to track crashes

Fix a test that shouldn’t have passed

Ensure stack traces show up in manual events

Fix crash logging not working in demo app

mokagio

I need to take a break, so I'm submitting this incomplete review.

So far everything looks good. I really only left nitpicks.

Automattic-Tracks-iOS.podspec

mokagio · 2021-01-20T00:31:30Z

Automattic-Tracks-iOS/Crash Logging/CrashLogging.swift

 import Sentry
 import CocoaLumberjack

+struct CrashLoggingInternals {


I like the idea of shoving all of the stuff that we cannot easily migrate into a dedicated place to make it easier to find and change or remove later 👍

I'm addicted to nested types recently. I can see this fit nicely as one

public class CrashLogging { /// Comment explaining why this is here... class Internals { // ... } // ... Internals.crashLogging = self // ... }

No need to implement it if you don't see the benefit. Just playing code golf.

mokagio · 2021-01-20T00:33:27Z

Automattic-Tracks-iOS/Crash Logging/CrashLogging.swift

-        // Only allow initializing this system once
-        guard !isStarted else { return }
-        isStarted = true
+        /// Validate the DSN ourselves before initializing, because the SentrySDK silently prints the error to the log instead of telling us if the DSN is valid


Suggested change

/// Validate the DSN ourselves before initializing, because the SentrySDK silently prints the error to the log instead of telling us if the DSN is valid

// Validate the DSN ourselves before initializing, because the SentrySDK silently prints the error to the log instead of telling us if the DSN is valid

I'm suggesting to go from /// to // because the former is to be used for documentation comments, but this one is not an header doc of a type or method.

the Sentry SDK silently prints the error to the log instead of telling us if the DSN is valid

Have you considered opening an issue with a feature suggestion for this.

Yeah, I like the internals thing a lot – done in 389e8c1

Regarding the /// vs // thing – WooCommerce started the trend of using the /// format for comments because they support markdown, links, etc (and are far smaller).

There's some of that already in the ExPlat stuff, so I've been aiming to use the /// format everywhere. However, I did have instances of // that should have been annotated with MARK:, so I've added that where appropriate.

Okay. Maybe I'm wrong about Apple's desired purpose for /// vs //.

Anyways, it's just a style thing 👌

Maybe I'm wrong about Apple's desired purpose

Probably not – we just abuse it for other things 😅

Automattic-Tracks-iOS/Crash Logging/CrashLogging.swift

Automattic-Tracks-iOS/UI/Crash Logging/CrashLoggingView.swift

mokagio · 2021-01-20T01:25:06Z

Automattic-Tracks-iOS/UI/Crash Logging/CrashLoggingView.swift

+                            switch sendErrorAndWaitStatus {
+                                case .none:
+                                    Group {} /// An empty view
+                                case .uploading:
+                                    Text("⏳")
+                                case .success:
+                                    Text("✅")
+                                case .error:
+                                    Text("⚠️")
+                            }


I'm a big fan of using enums to describe mutually exclusive states in the UI. Nicely done 😄

mokagio · 2021-01-20T01:27:09Z

Automattic-Tracks-iOS/UI/Crash Logging/CrashLoggingView.swift

+                Button("Send Test Crash", action: sendTestCrash)
+                Button("Send Test Event", action: sendTestEvent)
+                HStack {
+                    Button(action: sendErrorAndWait) {


I'm obviously over thinking this, given this is a debug view, but... as far as I can see, there is no logic to prevent running the action when the state is .uploading.

Agreed. I had to strongly resist the urge to over-build this because it is a demo app, so I'll stay strong and leave this for now.

Though it will bug me lol

mokagio · 2021-01-20T01:32:33Z

Automattic-Tracks-iOSTests/Tests/CrashLoggingTests.swift


-        XCTAssert(CrashLogging.sharedInstance.currentUser.email == testUser.email)
-        XCTAssert(CrashLogging.sharedInstance.cachedUser?.email == testUser.email)
+        XCTAssertEqual(testUser.email, event?.user?.email)


Nitpick. It's unclear in the signature, but the convention I've always seen used with XCTAssertEqual it to have the actual value as the first element and the expect as the second.

That's what the Apple examples do, too:

source

source.

Is there a different convention in place in this codebase?

No – that convention is correct!

Here, the testUser is the "constant" (ie – not the subject of the test) so the ordering is as expected.

Definitely something to nitpick – IMHO this is important for test readability so we should keep it consistent!!

Is it though? @mokagio 's suggestion is that the convention is XCTAssert(sut, expectedConstantValue), so the constant being as the second parameter. That's also always the convention I used I think.
So if the SUT is event?.user?.email and the testUser is the "constant" (= the expected value), then the order in your code is reversed, isn't it?

mokagio · 2021-01-20T01:34:24Z

TracksDemo/Podfile

  platform :ios, '12.0'

-  pod 'Automattic-Tracks-iOS', :path => '../'
+  pod 'Automattic-Tracks-iOS', path: '../'


🥳 it's the little things...

mokagio

Requesting changes only because of the demo project not building for iOS, which is an easy fix.

I tested on iOS and Mac with my puny Intel and it worked as described; I saw the events with my data on Sentry. I used my work email if you want to double check. 👏 👏

Love that we have a test application. There's a lot we can do to unit test the heck out of this library, but the Sentry integration will always need a bit of manual testing.

On tiny note: I couldn't find the iOS/tracks-demo folder mentioned in the description in the secrets store.

I left a lot of nitpicks, which you should take as what they are: just nitpicks.

In particular, I worry my many /// vs // suggestions will come across as overly pedantic. After adding the first couple, I felt the sunk cost pressure and kept on adding them 😳 I put them there to hopefully help with the conversion, but that's not a requirement for this PR to be merged.

TracksDemo/Shared/CrashLoggingDataSource.swift

TracksDemo/Shared/Secrets.example.swift

TracksDemo/TracksDemo/TracksDemo-Bridging-Header.h

mokagio · 2021-01-20T03:04:39Z

TracksDemo/TracksDemo.xcodeproj/project.pbxproj

-				IPHONEOS_DEPLOYMENT_TARGET = 10.0;
+				IPHONEOS_DEPLOYMENT_TARGET = 14.0;


The Podfile for the demo project sets 12.0 as the deployment target. We can't use that here because of SwiftUI, but should we make them meet in the middle at 13.0?

I can see an argument for running on the latest iOS only and not have to worry about backward compatibility. At the same time, Tracks is not restricted to iOS 14.0 only, so it might be beneficial for the demo app to run against as many compatible versions of iOS as reasonable - that is, iOS 13 so we can keep test the SwiftUI-related stuff.

Yeah seems reasonable – changed to iOS 13 in 9ed5b7a

mokagio · 2021-01-20T03:06:38Z

TracksDemo/TracksDemo.xcodeproj/project.pbxproj

-				MACOSX_DEPLOYMENT_TARGET = 10.14;
+				MACOSX_DEPLOYMENT_TARGET = 11.0;


Similar comment as the iOS 13.0 vs 14.0 above. Should we set this as 10.15 and do the same on the Podfile?

Addressed in 9ed5b7a, Podfile in 58f9012

mokagio · 2021-01-20T03:19:43Z

TracksDemo/TracksDemo/CrashLoggingRootViewController.swift

+    let crashLogging = CrashLogging(dataProvider: CrashLoggingDataSource())
+
+    init() {
+        crashLogging.start()


I had to update this to a brute for the iOS demo to build.

Suggested change

crashLogging.start()

_ = try! crashLogging.start()

Shoot, sorry about that – fixed in b3ba4d2 (in a slightly different way)

Slightly different but still effective 👍

TracksDemo/TracksDemo/CrashLoggingRootViewController.swift

Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>

jkmassel · 2021-01-22T06:03:04Z

@mokagio and @AliSoftware, thanks for the feedback – it should all be addressed now – ready for another look when you're ready 😃

One last change – I didn't realize the schemes weren't shared – I've added them to the repo in 901bf1d. That should fix the issue where in testing the "Send Test Event" wouldn't work (it needs to override the debug mode).

AliSoftware · 2021-01-22T15:38:26Z

Tested on iPhone Simulator (iPhone 11 Pro, iOS 13.5) and on Intel Mac. Was able to see both events in Sentry ("Send Test Event" + "Send Error and Wait") in both cases

I got this log in Xcode console each time I clicked the button when testing with the iPhone Simulator app. I'm not entirely sure it's expected to especially get the "Cancelling log file attachment"? 🤔

2021-01-22 16:25:22:594 TracksDemo[77872:5993852] 📜 Firing `beforeSend`
2021-01-22 16:25:22.594496+0100 TracksDemo[77872:5994015] 📜 Firing `beforeSend`
2021-01-22 16:25:22:594 TracksDemo[77872:5993852] 📜 This is a debug build
2021-01-22 16:25:22.594710+0100 TracksDemo[77872:5994015] 📜 This is a debug build
2021-01-22 16:25:22:594 TracksDemo[77872:5993852] 📜 Cancelling log file attachment – Event Logging is not initialized
2021-01-22 16:25:22.594901+0100 TracksDemo[77872:5994015] 📜 Cancelling log file attachment – Event Logging is not initialized

I also still got a diff on the Podfile.lock even after cleaning my Pods/ and working copy and redoing a pod install… but this time a different one.

git clean + pod install + git diff

$ git clean -dxf
Removing Pods/
Removing Shared/Secrets.swift
Removing TracksDemo.xcodeproj/xcuserdata/
Removing TracksDemo.xcworkspace/xcuserdata/
                                                                                                                                                                                                                                
$ cp ~/.mobile-secrets/iOS/tracks-demo/Secrets.swift Shared/Secrets.swift         
                                                                                                                                                                                                                                
$ be pod install
Analyzing dependencies
Downloading dependencies
Installing Automattic-Tracks-iOS (0.7.1)
Installing CocoaLumberjack (3.4.2)
Installing Reachability (3.2)
Installing Sentry (6.1.2)
Installing Sodium (0.9.1)
Installing UIDeviceIdentifier (1.1.4)
Generating Pods project
Integrating client project
Pod installation complete! There is 1 dependency from the Podfile and 6 total pods installed.
                                                                                                                                                                                                                                
$ gst
On branch update/sentry-to-v6
Your branch is up to date with 'origin/update/sentry-to-v6'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   Podfile.lock

no changes added to commit (use "git add" and/or "git commit -a")

$ git diff

diff --git a/TracksDemo/Podfile.lock b/TracksDemo/Podfile.lock
index 712c51e..f701edb 100644
--- a/TracksDemo/Podfile.lock
+++ b/TracksDemo/Podfile.lock
@@ -1,5 +1,5 @@
 PODS:
-  - Automattic-Tracks-iOS (0.7.0):
+  - Automattic-Tracks-iOS (0.7.1):
     - CocoaLumberjack (~> 3)
     - Reachability (~> 3)
     - Sentry (~> 6)
@@ -34,7 +34,7 @@ EXTERNAL SOURCES:
     :path: "../"
 
 SPEC CHECKSUMS:
-  Automattic-Tracks-iOS: 01fd960098c3e6e2822531c1eeb6d03dbddd865c
+  Automattic-Tracks-iOS: 7ac2cc974ec22d1273bd8df9eae0cfb3193d959d
   CocoaLumberjack: db7cc9e464771f12054c22ff6947c5a58d43a0fd
   Reachability: 33e18b67625424e47b6cde6d202dce689ad7af96
   Sentry: 3a6c3aeb258b297a4be08d8960955ecc722331f3

jkmassel · 2021-01-22T16:46:37Z

I got this log in Xcode console each time I clicked the button when testing with the iPhone Simulator app. I'm not entirely sure it's expected to especially get the "Cancelling log file attachment"? 🤔

That's expected – the encrypted logging system isn't injected into CrashLogging, so we log the fact that we're not uploading an encrypted log for this crash. This is especially important when debugging in-app so devs aren't confused why no logs are showing up and there's no logId associated with the Sentry event.

jkmassel · 2021-01-22T16:49:59Z

Thanks for the detailed instructions – I was able to replicate that issue this time!

This time I think it was caused by the merge with develop?

But should be resolved now 😅

AliSoftware

Took some time to also re-read the code.

Overall LGTM; I left a couple of nits but that are optional and shouldn't block that PR.
The other nits about XCTAssertEqual(sut, expected) as convention also still apply (but are also only nits, so…)

I still left one question about the assumption for active thread that might need checking and validating before merging though (but didn't want to keep the "Request Changes" status just for this one.) so I'll let you address them before hitting the button.

AliSoftware · 2021-01-22T23:56:07Z

Automattic-Tracks-iOS/Crash Logging/SentryClient+Shim.m

+    if(eventWithStackTrace.threads.firstObject.stacktrace != nil) {
+        eventWithStackTrace.stacktrace = eventWithStackTrace.threads.firstObject.stacktrace;
+    }


I am guessing this is based on the assumption that the real crash is always guaranteed to be in the threads.firstObject? I mean I guess that makes sense given that that's probably the thread that was active when the crash occurred, but is the currently active thread always the one that is first in this array though? 🤔

Yeah, for now it's a reasonable assumption – this functionality is designed for logging crashes on startup so that we can log the message before another crash happens. The startup stuff is all currently on the main thread, so this should work fine for quite some time.

As part of the core project to re-work app initialization, I'm hoping to kill this code and be able to put up a UI for something like "the app crashed last time – would you like to try again or reset?" and in that time Sentry could do its thing and we could kill this code.

put up a UI for something like "the app crashed last time – would you like to try again or reset?"

That would be such a great thing to have!

Automattic-Tracks-iOS/Crash Logging/SentryEventSerializer.swift

Automattic-Tracks-iOS/Crash Logging/SentryClient+Shim.m

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

loremattei

I haven't re-reviewed the code as I see that @AliSoftware and @mokagio already did it.

I tested on a M1 Mac following the instructions:
☑️ Try building for Any iOS Device
☑️ Try building for My Mac, and Any Mac (Apple Silicon, Intel)

and everything (including archiving it) was successful.
I've seen a few warnings, but I suppose they are expected at this point.

This feedback is addressed!

jkmassel added the enhancement label Jan 16, 2021

jkmassel self-assigned this Jan 16, 2021

jkmassel marked this pull request as draft January 16, 2021 06:26

jkmassel force-pushed the update/sentry-to-v6 branch from db8106c to 87540f7 Compare January 16, 2021 06:30

jkmassel mentioned this pull request Jan 16, 2021

Update the Sentry SDK to 5.x #131

Closed

jkmassel force-pushed the update/sentry-to-v6 branch 3 times, most recently from be0b60c to 270a1d8 Compare January 16, 2021 06:37

This was referenced Jan 18, 2021

Update Sentry (adding in manual event sending) #159

Closed

[EPIC] – Adds UI for Crash Logging, Encrypted Logs, and Sentry #165

Closed

jkmassel force-pushed the update/sentry-to-v6 branch 2 times, most recently from d6306ad to ed1c788 Compare January 18, 2021 19:34

jkmassel requested review from AliSoftware, astralbodies, loremattei and mokagio January 18, 2021 21:59

jkmassel commented Jan 18, 2021

View reviewed changes

jkmassel added 5 commits January 18, 2021 15:07

Upgrade Sentry to 6.0.12

a103a21

Fix a test that shouldn’t have passed

Use the Sentry API directly

5977db7

Ensure stack traces show up in manual events

Add Sentry Testing UI

bd1f786

Add Sentry Debugging to iOS Demo

b7aeb2c

Fix crash logging not working in demo app

Add Sentry Debugging to macOS Demo

6fb784b

jkmassel force-pushed the update/sentry-to-v6 branch from 7d5089a to 6fb784b Compare January 18, 2021 22:07

jkmassel marked this pull request as ready for review January 18, 2021 22:09

mokagio mentioned this pull request Jan 19, 2021

Switch back to Sodium's original library & update to 0.9.0 #157

Closed

mokagio reviewed Jan 20, 2021

View reviewed changes

mokagio previously requested changes Jan 20, 2021

View reviewed changes

mokagio mentioned this pull request Jan 20, 2021

Update project configuration and pods to build for Apple Silicon Automattic/simplenote-macos#802

Merged

AliSoftware mentioned this pull request Jan 20, 2021

Update Sparkle to version 1.24.0 to support Apple Silicon Automattic/simplenote-macos#803

Closed

mokagio self-requested a review January 21, 2021 18:47

jkmassel and others added 10 commits January 21, 2021 22:28

Fix CrashLogging header

e15aa38

Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>

Avoid some needless work

b0eedd7

Increase demo project compatiblity

9ed5b7a

Use explicit error variable

baec210

Rename tryAddingStackTrace to addStackTrace

20af629

Add a reference to the Sentry SDK issue

c6a751b

Align podfile versioning with project

58f9012

Add more inline documentation

6d6c33f

Fire Captain Obvious

a571403

Add a better error description for SentryTestError

9b9c107

Merge branch 'develop' into update/sentry-to-v6

75895bb

jkmassel requested a review from AliSoftware January 22, 2021 06:07

Add shared schemes

901bf1d

Update Podfile.lock

febb3c6

AliSoftware approved these changes Jan 23, 2021

View reviewed changes

jkmassel and others added 4 commits January 22, 2021 21:15

Update Automattic-Tracks-iOS/Crash Logging/SentryEventSerializer.swift

ec502d6

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

Update Automattic-Tracks-iOS/Crash Logging/SentryClient+Shim.m

ea5f716

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

Update Automattic-Tracks-iOS/Crash Logging/SentryClient+Shim.m

c13a5ac

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

Update Automattic-Tracks-iOS/Crash Logging/SentryClient+Shim.m

ce23638

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

loremattei approved these changes Jan 26, 2021

View reviewed changes

jkmassel merged commit 37477d6 into develop Jan 26, 2021

jkmassel deleted the update/sentry-to-v6 branch January 26, 2021 17:36

spencertransier mentioned this pull request Nov 11, 2023

Add configure automation for Secrets.swift #266

Open

	/// Validate the DSN ourselves before initializing, because the SentrySDK silently prints the error to the log instead of telling us if the DSN is valid
	// Validate the DSN ourselves before initializing, because the SentrySDK silently prints the error to the log instead of telling us if the DSN is valid

		IPHONEOS_DEPLOYMENT_TARGET = 10.0;
		IPHONEOS_DEPLOYMENT_TARGET = 14.0;

		MACOSX_DEPLOYMENT_TARGET = 10.14;
		MACOSX_DEPLOYMENT_TARGET = 11.0;

Conversation

jkmassel commented Jan 16, 2021 • edited by AliSoftware Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkmassel Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkmassel commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Jan 22, 2021

jkmassel commented Jan 16, 2021 •

edited by AliSoftware

Loading

jkmassel Jan 20, 2021 •

edited

Loading

mokagio Jan 20, 2021 •

edited

Loading

jkmassel commented Jan 22, 2021 •

edited

Loading

AliSoftware left a comment •

edited

Loading

jkmassel Jan 23, 2021 •

edited

Loading