Closed Bug 1369514 Opened 9 years ago Closed 8 years ago

Sync telemetry shouldn't ignore TelemetryController.submitExternalPing promise

Categories

(Firefox :: Sync, enhancement, P3)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: tcsc, Unassigned)

References

(Depends on 1 open bug)

Details

Found while looking into bug 1369186. This function call returns a promise: http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/services/sync/modules/telemetry.js#472 We never handle this promise, and from a quick check, other callers imply we should be. I don't know what cases would cause it to reject (aside from the ones that occur at the start of that function, like type errors and such), I suspect we'd want to attempt to resubmit later in cases where we reject? That's assuming we can meaningfully handle the rejection, which it's possible we can't... I think at the very least we should do some logging though.
Priority: -- → P1
Assignee: nobody → tchiovoloni
I see we need to properly update the doc comment [1] for the rejection cases. They are: - submitting an invalid ping type string - submitting a payload that is not an object - submitting a ping after shutdown Those are mostly useful when you are manually testing your new ping (and for tests). I don't think there are useful runtime actions you can take on those in normal operations. Everything else is the responsibility of the Telemetry system. 1: https://dxr.mozilla.org/mozilla-central/rev/cad53f061da634a16ea75887558301b77f65745d/toolkit/components/telemetry/TelemetryController.jsm#209
Filed bug 1370460 on making this more clear.
Looking into this, i see that rejections don't make sense for this API. I'm changing this in bug 1370460 into throwing if the API is used incorrectly. That means that the returned promise is for testing only and you should not wait for it.
Depends on: 1370460
Assignee: tchiovoloni → nobody
Priority: P1 → P3
(In reply to Georg Fritzsche [:gfritzsche] from comment #3) > That means that the returned promise is for testing only and you should not > wait for it. Sounds like this is a WONTFIX, then. :-)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.