[NEEDS iOS TESTING] API errors in Sentry, v2

[rk-for-zulip]

Alternate to #4086. Started life as a continuation of #4086, but has grown sufficiently divergent that it probably shouldn't be unified with that one.

Sentry events created by this code (by forcing 200 OK responses to be seen as errors) can be seen here. (Note that some early events have the Zulip error code sent up as the HTTP status; this may be ignored, as may any earlier events not titled "random error string".)

Notes about the patchset:

• The first three commits are Remove objectToParams #4090, and should be reviewed there. (Hence the "blocked-on-other-work" label.) (Done!)
• The commit api: Stop marking failure with breadcrumbs. can be omitted, but I'd suggest retaining it.
• The final commit is sort of unrelated, but saved me several times from accidentally pushing onto an existing release. I recommend adopting something like it.
• Although there's a callout to View uploaded files without needing to be logged in to the realm in the browser. #4089, neither of these PRs need to block the other; the notional "merge conflict" can be resolved as a relatively-small followup commit.

Fixes #3864.

[gnprice]

Thanks!

I think this bucketing / disaggregation strategy will work OK.

In apiFetch I'm a little concerned about the many different things named more or less "params". Some of that is pre-existing, but there's a growth here in types with similar names to each other, also often defined by indirection so it takes several steps to look up what's in them.

One thing in particular I'm sure we can improve is this description in terms of apiFetch:

/**
 * Additional parameters passed to apiFetch.
 *
 * This is a strict subset of `RequestOptions`, the actual argument type of
 * `fetch()`.
 */
type FetchParams = {| method: string, body?: string | FormData |};

when there's another type ApiFetchParams which is different.

 * @param isSilent: True if this fetch should not trigger the network activity
 *    indicator on iOS. Should be `true` only for long-polling API calls.
 *    Deprecated as of iOS 13.

This is interesting new information! Can you link to your source for the deprecation? Probably best added in a separate commit, too.

The build.gradle change at the end looks helpful. Let's move the bulk of it to a function, though -- so the code in that central place can be short, like

        // Unofficial builds normally don't report to Sentry.  If this one will
        // (say, to test Sentry), mark it as distinct from an official build.
        if (project.hasProperty('sentry') && !project.hasProperty('signed')) {
            versionName += localVersionSuffix()
        }

[rk-for-zulip]

In apiFetch I'm a little concerned about the many different things named more or less "params".

Ain't that the truth. 😑 I'd say it was much worse before, though – at least now the typechecker will yell at you if you get them crossed.

One thing in particular I'm sure we can improve is this description in terms of apiFetch:

/**
 * Additional parameters passed to apiFetch.

... not least because it was intended to read "Additional parameters passed to fetch()", not "apiFetch". 😓 I remember correcting that, too – it must have gotten lost in one of the many rebase-shuffles. (If that's all that got lost, I'll be happy.)

This is interesting new information! Can you link to your source for the deprecation? Probably best added in a separate commit, too.

Apple's own documentation marks it as deprecated. I haven't found any official note that the deprecation occurred in iOS 13 specifically – Apple's stopped updating "What's New in iOS", alas – though tweets and SO posts are easily found.

The link probably belongs in src/utils/networkActivity.js, regardless.

The build.gradle change at the end looks helpful. Let's move the bulk of it to a function, though

👍

[gnprice]

at least now the typechecker will yell at you if you get them crossed.

True!

I haven't found any official note that the deprecation occurred in iOS 13 specifically – Apple's stopped updating "What's New in iOS", alas – though tweets and SO posts are easily found.

Ah, that's one of their lovely "archived" docs pages.

IME when I've found some Apple docs like that, there is a new up-to-date version -- they just didn't bother to link to it, or migrate the old stuff to keep it all in one place.

In this instance, googling [what's new in ios] finds this page on their new docs site:
https://developer.apple.com/documentation/ios_ipados_release_notes

which has detailed release notes that pick up where that archived page left off.

(Dunno if this particular deprecation is mentioned there, though.)

The link probably belongs in src/utils/networkActivity.js, regardless.

Sure. Then I think the deprecation can be left out in the other place -- it's not really actionable from the perspective of a caller of apiCall.

[rk-for-zulip]

In this instance, googling [what's new in ios] finds this page on their new docs site:
https://developer.apple.com/documentation/ios_ipados_release_notes

[...] (Dunno if this particular deprecation is mentioned there, though.)

Sadly, it's not: that page seems to be generally less comprehensive than the older version was. I did eventually find a mention here, though. (Until they silently remove it, at least. Perhaps I should link to the Internet Archive's version.)

Sure. Then I think the deprecation can be left out in the other place -- it's not really actionable from the perspective of a caller of apiCall.

👍

[gnprice]

I did eventually find a mention here, though. (Until they silently remove it, at least. Perhaps I should link to the Internet Archive's version.)

Cool. FTR at least for this thread's sake, here's what it says:

The network activity indicator is deprecated in iOS 13 and on devices with edge-to-edge displays. In iOS 12 and earlier, and on devices without edge-to-edge displays, a network activity indicator spins in the status bar at the top of the screen as networking occurs. It disappears when networking is complete. This indicator looks just like an activity indicator and is noninteractive.

Probably we should just keep setting it until we no longer support iOS versions older than 13, unless we learn that it's causing some kind of trouble on devices where it doesn't have anything to do.

[rk-for-zulip]

Probably we should just keep setting it until we no longer support iOS versions older than 13, unless we learn that it's causing some kind of trouble on devices where it doesn't have anything to do.

I expect it to be only to us, in that every byte of code has a cost. But I don't actually have a strong opinion on that; feel free to trim the parenthetical off that last's commit message, if you like.

The more important thing – which I've just indicated in the PR title – is that ⚠️ this patchset needs to be tested on iOS. ⚠️ I obviously haven't been able to do so, and it contains a Sentry upgrade with a minor version bump.