Closed Bug 1364899 Opened 7 years ago Closed 7 years ago

Add passing of sync failures to the sync ping

Categories

(Firefox for iOS :: Telemetry, enhancement, P1)

Other
iOS
enhancement

Tracking

()

RESOLVED FIXED
Iteration:
1.22
Tracking Status
fxios 9.0 ---

People

(Reporter: sleroux, Assigned: sleroux)

References

Details

(Whiteboard: [MobileCore])

Attachments

(1 file)

On iOS, when an sync engine has failed/errored, we throw a FatalError/SyncError type with a custom message. We should add this to the sync ping in place of the desktop failureReason as it gives more context what the errors the iOS client is encountering. It's worth noting that we already handle the normal .partial and .notStarted states.
Status: NEW → ASSIGNED
Iteration: --- → 1.21
Iteration: 1.21 → 1.22
Priority: P2 → P1
Comment on attachment 8868214 [details] [review]
Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2729

LGTM
Attachment #8868214 - Flags: review?(jhugman) → review+
Hey Mark,

Before I land this, I wanted to run the change in format for the sync ping's `failureReason` field by you. We don't have the same kind of failure states as desktop so trying to conform to the existing spec doesn't make much sense. Would it be possible to just send a string for this error? We could also incorporate a kind of identifier bucketing categories of errors for easier analysis down the road.
Flags: needinfo?(markh)
(In reply to Stephan Leroux [:sleroux] from comment #3)
> Before I land this, I wanted to run the change in format for the sync ping's
> `failureReason` field by you. We don't have the same kind of failure states
> as desktop so trying to conform to the existing spec doesn't make much
> sense.

Yeah, agreed - as you are discovering, it's not really a "spec" at all :)

> Would it be possible to just send a string for this error? We could
> also incorporate a kind of identifier bucketing categories of errors for
> easier analysis down the road.

I think that for failureReason we'd prefer an object - possibly like {name, error} (or maybe {name, %anything%} - Thom probably has clearer thoughts...
Flags: needinfo?(markh) → needinfo?(tchiovoloni)
(In reply to Mark Hammond [:markh] from comment #4)
> I think that for failureReason we'd prefer an object - possibly like {name,
> error} (or maybe {name, %anything%} - Thom probably has clearer thoughts...

I think this is basically what we want. It's what the scala code in the backend turns it into as well. Something like {name: string, error: string}, is probably the best option. 

Using name == "unexpectederror" would allow the current code to parse these pings without needing to update the scala (not that doing so would be a terribly big deal).

As it is, using a string directly will very likely cause the Scala code processing the pings to skip these pings, unfortunately. (It might even cause it to throw/crash, but I don't think so).
Flags: needinfo?(tchiovoloni)
Thanks :markh, :tcsc - I've updated the patch to set failureReason to a dictionary:

"name": iOS Error Enum name
"error": Description of the error

I could set name to "unexpectederror" but that doesn't really have any meaning for us on iOS.
(In reply to Stephan Leroux [:sleroux] from comment #6)
> Thanks :markh, :tcsc - I've updated the patch to set failureReason to a
> dictionary:
> 
> "name": iOS Error Enum name
> "error": Description of the error
> 
> I could set name to "unexpectederror" but that doesn't really have any
> meaning for us on iOS.

The scala code used to extract these is at https://github.com/mozilla/telemetry-batch-view/blob/master/src/main/scala/com/mozilla/telemetry/views/SyncView.scala#L234, and sadly the list of "names" is hard-coded there. I'm not sure how iOS works, but if "Description of the error" might be localized it can make it more difficult to analyse. We could change the scala code, but it sounds like it might be simpler all-round as 

"name": "unexpectederror",
"error": iOS Error Enum name
What I can do is go through all our error states and try to map them best to the error types the server expects. I can see some pretty easy connections between sqlerror, httperror, othererror and unexpectederror. I think we'll be able to do something like:

"name": (httperror || sqlerror || othererror || unexpectederror),
"error": iOS Error Enum name.
(In reply to Stephan Leroux [:sleroux] from comment #8)
> What I can do is go through all our error states and try to map them best to
> the error types the server expects. I can see some pretty easy connections
> between sqlerror, httperror, othererror and unexpectederror. I think we'll
> be able to do something like:
> 
> "name": (httperror || sqlerror || othererror || unexpectederror),
> "error": iOS Error Enum name.

This is probably the best option.

Unfortunately, it's worth noting that the desktop code isn't consistent with the second property name (or it is, just not in a way that's helpful to other platforms implementing the sync ping).  E.g. "httperror", "sqlerror" (and "nserror") use "code" (since it's an integer error code), while "othererror" and "unexpectederror" use "error" (since it's a string). See https://dxr.mozilla.org/mozilla-central/source/services/sync/tests/unit/sync_ping_schema.json#152-199 specifically.

Reading the scala, it looks like we will handle integers or strings in either field, but we only check "code" for httperror/sqlerror and "error" for the others.

(I'm not certain that I'm explaining this particularly well, feel free to ask for clarification)
And even with all the above said, changing the scala code really isn't too difficult, so if things start getting too smelly we should keep that option on the table.
Agreed, it's totally possible that the right choice here is to accept "error" as well as "code" in the Scala.
master f643af56811da01fc66570e572aa84a3ad305873
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: