Use Sentry `flush` to send events immediately #232

mokagio · 2022-10-17T02:13:09Z

This PR removes our custom logic to implement the "send all enqueue events and wait" functionality in favor of using the new Sentry's native flush method.

One big disadvantage of this approach is that we lose the precise callback management, because flush is blocking for as long as it takes to send all events or for a given timeout to run out.

I used the demo app to verify our custom implementation works and the events it sends reach Sentry. In the screenshot below, the top two events have been sent from this branch. Also notice the third and fourth events have "(no value)" for release and environment. That's a bug introduced at some point when migrating to version Sentry version 7. I haven't looked into it yet.

I'm of two minds on whether to go for using flush under the hood. I really dislike how flush doesn't provide a callback out of the box, and how it results in a less precise UI state handling on our end. At the same time, relying on Sentry's native code instead of implementing our own logic accessing their private API simplifies and de-risks the library.

I'd like other folks input on this, but I'm tempted to adopt this implementation on the basis that logErrorAndWait, logErrorImmediately, and logErrorsImmediately are used very sparingly by the library's clients. Namely, in one occasion in Jetpack/WordPress and in two occasions in WooCommerce](https://github.com/woocommerce/woocommerce-ios/search?q=logFatalErrorAndExit).

Sources/Remote Logging/Crash Logging/CrashLogging.swift

mokagio · 2022-10-19T04:24:48Z

Sources/UI/Crash Logging/CrashLoggingView.swift

-                }
+            try crashLogging.logErrorImmediately(
+                error,
+                userInfo: ["custom-userInfo-key": "custom-userInfo-value"]


I realized we never "tested" sending custom information, so I added it. Here's the result:

mokagio · 2022-10-19T04:25:40Z

Sources/UI/Crash Logging/CrashLoggingView.swift

+                error,
+                userInfo: ["custom-userInfo-key": "custom-userInfo-value"]
+            ) {
+                sendErrorAndWaitStatus = .success


If we decide to adopt flush, it might be appropriate to update this enum to remove the error-tracking state, since there's no longer any error information with flush.

mokagio · 2022-10-19T04:27:11Z

Sources/Remote Logging/Crash Logging/CrashLogging.swift

@@ -168,54 +171,24 @@ public extension CrashLogging {
    }

    /// Sends an `Event` to Sentry and triggers a callback on completion
-    func logErrorImmediately(_ error: Error, userInfo: [String: Any]? = nil, level: SentryLevel = .error, callback: @escaping (Result<Bool, Error>) -> Void) throws {
+    func logErrorImmediately(_ error: Error, userInfo: [String: Any]? = nil, level: SentryLevel = .error, callback: @escaping () -> Void) throws {


Surprisingly, I'm not getting any errors or warnings from the compiler because of these methods still having throws despite there not being any genuine try call in the chain.

If we commit to flush, we should remove throws as well.

Sources/Remote Logging/Crash Logging/CrashLogging.swift

mokagio · 2022-10-24T03:25:21Z

Sources/Remote Logging/Crash Logging/CrashLogging.swift

+        queue.async {
+            SentrySDK.flush(timeout: CrashLogging.eventsFlushingTimeout)
+            callback()
+        }


@crazytonyli what do you think of this as an implementation of your suggestion to make the call in a background queue?

In regards to your suggestion to add a wait: Bool parameter, are you thinking of something along the lines of WordPress iOS where we use a Bool parameter to decide whether to call context.performAndWait(_:) or context.perform(_:)?

In this, I'm thinking if wait == true we wouldn't run the code in queue.async.

Yes, so that we don't need to use semaphore like the original implementation 😄

😕 The new diff makes this conversation odd because the code it referred to has moved. Anyways... @crazytonyli I had an initial go at adding a wait parameter.

I did this in the form of a private func for the other methods to call.

private func logErrorsImmediately( _ errors: [Error], userInfo: [String: Any]? = nil, level: SentryLevel = .error, andWait wait: Bool, callback: @escaping () -> Void ) {

I didn't modify the original method signatures to avoid a breaking change, but also because I wasn't sure how it would have felt to have a andWait:, together with a callback:. If wait == true, then there's no need for a callback because the code that would run in there essentially runs once the method has finished anyway.

This is based on the suggestion at #232 (comment)

Sources/Remote Logging/Crash Logging/CrashLogging.swift

crazytonyli · 2022-10-31T07:28:27Z

Sources/Remote Logging/Crash Logging/CrashLogging.swift

+        if wait {
+            flushEventThenCallCallback()
+        } else {
+            let queue = DispatchQueue(label: "com.automattic.tracks.flushSentry", qos: .background)


Any reason why using a custom queue instead of DispatchQueue.global? Also, any reason why choose .background QoS, instead of the default one? (I almost always use the default QoS since I've heard scary things about QoS but don't really understand it 😸 )

I followed the suggestion from one of the Sentry engineers from here.

I trust your advice on this matter, though. I'll update.

Done in Automattic/Automattic-Tracks-iOS@8a0c84b (#232)

crazytonyli · 2022-10-31T07:32:29Z

Sources/Remote Logging/Crash Logging/CrashLogging.swift

-            let didSucceed = 200...299 ~= (urlResponse as! HTTPURLResponse).statusCode
-            callback(.success(didSucceed))
-        }.resume()
+    func logErrorsImmediately(_ errors: [Error], userInfo: [String: Any]? = nil, level: SentryLevel = .error, callback: @escaping () -> Void) throws {


I had a quick look, it appears we can't get the result of flush call, is that correct? I think it would be good to at least have a "success or not" result, if we can't get a concrete error. But I guess there isn't anything we can do if Sentry doesn't expose such API.

Correct. See the details given here. It's quite the let down. There must be a reason for the lack of callback in their unified API rationale, but to be honest I haven't read that documentation.

Notice that, because `flush` doesn't support callbacks but instead blocks the main thread up to a given timeout interval, we had to remove our more refined callback mechanism and UI state management logic.

This code was all custom interaction with the Sentry SDK for the purpose of extracting information to add to events sent to Sentry "manually" via HTTP requests. Now that we use Sentry's native method for that, all the information tracking is handled by the Sentry SDK.

This is based on the suggestion at #232 (comment)

The word "events" made me think it ought to belong as part of `EventLogging` which was confusing because performance monitoring is currently implemented via Sentry (crash logging) instead.

See discussion at: #232 (comment)

…tatus` We no longer have error handling because of Sentry's `flush`.

None of the code left from the `flush` adoption threw errors.

The change in the `SendErrorAndWaitStatus` `enum` is not mentioned because the type is `internal`.

mokagio force-pushed the mokagio/sentry-flush branch from f927fc2 to 84ce7c3 Compare October 17, 2022 02:14

mokagio mentioned this pull request Oct 17, 2022

Dispatch an UI update event in the macOS demo app from the main thread #231

Merged

mokagio force-pushed the mokagio/sentry-flush branch from 84ce7c3 to 53fd031 Compare October 19, 2022 03:14

mokagio mentioned this pull request Oct 19, 2022

Add missing icons to demo app to address Xcode warning #233

Merged

mokagio force-pushed the mokagio/sentry-flush branch 2 times, most recently from 0a375bb to 82475d1 Compare October 19, 2022 04:23

mokagio commented Oct 19, 2022

View reviewed changes

Sources/Remote Logging/Crash Logging/CrashLogging.swift Outdated Show resolved Hide resolved

mokagio commented Oct 19, 2022

View reviewed changes

mokagio requested review from jkmassel and crazytonyli October 19, 2022 04:27

crazytonyli requested changes Oct 19, 2022

View reviewed changes

Sources/Remote Logging/Crash Logging/CrashLogging.swift Outdated Show resolved Hide resolved

mokagio commented Oct 24, 2022

View reviewed changes

mokagio added a commit that referenced this pull request Oct 27, 2022

Refactor the various logError... implementation with a common one

2a89afa

This is based on the suggestion at #232 (comment)

mokagio changed the title ~~Experiment: Use Sentry flush to send events immediately~~ Use Sentry flush to send events immediately Oct 31, 2022

mokagio marked this pull request as ready for review October 31, 2022 05:06

mokagio requested a review from crazytonyli October 31, 2022 05:07

crazytonyli reviewed Oct 31, 2022

View reviewed changes

mokagio added 8 commits November 7, 2022 13:06

Bump Sentry minimum requirement to 7.26.0

d723e4d

Use Sentry's native flush to force sending events

1e233b6

Notice that, because `flush` doesn't support callbacks but instead blocks the main thread up to a given timeout interval, we had to remove our more refined callback mechanism and UI state management logic.

Add a userInfo value when testing logErrorImmediately in demo app

450da05

Move call to Sentry's flush to a background queue

111fa70

Refactor the various logError... implementation with a common one

2adfbc5

This is based on the suggestion at #232 (comment)

Rename eventsFlushTimeout to flushTimeout

56a83d2

The word "events" made me think it ought to belong as part of `EventLogging` which was confusing because performance monitoring is currently implemented via Sentry (crash logging) instead.

Make flushTimeout a configurable instance property

30a873e

mokagio force-pushed the mokagio/sentry-flush branch from 2a89afa to 30a873e Compare November 7, 2022 16:14

Flush Sentry events on global DispatchQueue instead of a custom one

8a0c84b

See discussion at: #232 (comment)

mokagio requested a review from crazytonyli November 7, 2022 16:28

crazytonyli approved these changes Nov 8, 2022

View reviewed changes

mokagio added 4 commits November 8, 2022 10:42

Add CHANGELOG.md entry for the changes due to Sentry flush

7e785f2

Replace success and error cases with done in `SendErrorAndWaitS…

dfff1e3

…tatus` We no longer have error handling because of Sentry's `flush`.

Remove unused throws from logError[s]Immediately

14ac5ea

None of the code left from the `flush` adoption threw errors.

Add CHANGELOG.md entry regarding the throws removal

7ce8539

The change in the `SendErrorAndWaitStatus` `enum` is not mentioned because the type is `internal`.

mokagio mentioned this pull request Nov 8, 2022

Update CrashLoggingView after adopting Sentry flush, and remove dead throws #236

Merged

1 task

Update CrashLoggingView for Sentry flush & remove throws (#236)

c7f1bff

mokagio merged commit 8744936 into trunk Nov 9, 2022

mokagio deleted the mokagio/sentry-flush branch November 9, 2022 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Sentry `flush` to send events immediately #232

Use Sentry `flush` to send events immediately #232

mokagio commented Oct 17, 2022 •

edited

Loading

mokagio Oct 19, 2022

mokagio Oct 19, 2022

mokagio Oct 19, 2022

mokagio Oct 24, 2022

mokagio Oct 24, 2022

crazytonyli Oct 26, 2022

mokagio Oct 27, 2022

crazytonyli Oct 31, 2022

mokagio Nov 7, 2022

mokagio Nov 8, 2022

crazytonyli Oct 31, 2022

mokagio Nov 7, 2022

Use Sentry flush to send events immediately #232

Use Sentry flush to send events immediately #232

Conversation

mokagio commented Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use Sentry `flush` to send events immediately #232

Use Sentry `flush` to send events immediately #232

mokagio commented Oct 17, 2022 •

edited

Loading