Closed Bug 1150115 Opened 5 years ago Closed 4 years ago

Update the API for the remote about:healthreport content

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

(Whiteboard: [uplift])

Attachments

(1 file, 5 obsolete files)

We need to update the API used by about:healthreports remote content to keep working with the unification changes.

We will need to support both the old and the new form for now to allow for a preffable switchover depending on a later go/no-go decision.
Attached file API sketch (obsolete) —
This sketches out the new API parts.
Assignee: nobody → gfritzsche
Attachment #8586871 - Flags: feedback?(benjamin)
Depends on: 1150134
Actually, let's drop the version part.
We should probably just use a version URL to request the content:
> That server-side content is currently requested from a non-version URL:
> datareporting.healthreport.about.reportUrl;https://fhr.cdn.mozilla.net/%LOCALE%/
> ... we need to include at least version/build information in that URL.
Blocks: 1150491
Comment on attachment 8586871 [details]
API sketch

RequestPingList:

In the resulting ping list, is the submissionDate an accurate timestamp or does it just have daily granularity?

RequestPingData:

Is there a reason for batching all of the UUIDs together instead of requesting each one separately? I expect doing each one separately would be fine and make the API simpler.

Also, is the ping ID sufficient data for fetching a ping? Since we've decided to save the pings in dated directories, are we encoding the date in the ping ID somehow for easy lookup?

RequestCurrentEnvironment:

I think you should expose the current environment object.

The long-term API for this is MozSelfSupport.webidl? The message-based API is a temporary shim to the real self-support API while we get signing working.

I expect that in webidl, this would be something like:

Promise<sequence<object>> getTelemetryPingList();
Promise<object> getTelemetryPing(DOMString pingID);
Promise<object> currentEnvironment();
Attachment #8586871 - Flags: feedback?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Comment on attachment 8586871 [details]
> API sketch
> 
> RequestPingList:
> 
> In the resulting ping list, is the submissionDate an accurate timestamp or
> does it just have daily granularity?

This will be full timestamp/ms precision.

> RequestPingData:
> 
> Is there a reason for batching all of the UUIDs together instead of
> requesting each one separately? I expect doing each one separately would be
> fine and make the API simpler.

Ok, thinking about this, we should probably send the responses individually and add at least a failure notification.
For the request, i think it makes more sense to allow requesting an array of UUIDs - it's basically the same work and allows for much less round-trips (given that i expect the remote content to always request N pings anyway).

> Also, is the ping ID sufficient data for fetching a ping? Since we've
> decided to save the pings in dated directories, are we encoding the date in
> the ping ID somehow for easy lookup?

We had a discussion on thursday about bug 1137252 and related issues: 
We decided to store pings as <YYYY-MM>/<TIMESTAMP>.<UUID>.<type>.json, which covers our different concerns.

> RequestCurrentEnvironment:
> 
> I think you should expose the current environment object.

Good point.

> The long-term API for this is MozSelfSupport.webidl? The message-based API
> is a temporary shim to the real self-support API while we get signing
> working.
> 
> I expect that in webidl, this would be something like:
> 
> Promise<sequence<object>> getTelemetryPingList();
> Promise<object> getTelemetryPing(DOMString pingID);
> Promise<object> currentEnvironment();

Yes, that will be much nicer.
Attached file API sketch (obsolete) —
Attachment #8586871 - Attachment is obsolete: true
Attachment #8588550 - Flags: feedback?(benjamin)
Status: NEW → ASSIGNED
> For the request, i think it makes more sense to allow requesting an array of
> UUIDs - it's basically the same work and allows for much less round-trips
> (given that i expect the remote content to always request N pings anyway).

Why is the number of round-trips a concern? Assuming you agree with the MozSelfSupport API below, why wouldn't we make the message-based API an exact reflection of that? I suggested having each one be separate so that you could represent each one as a separate promise.

> > Also, is the ping ID sufficient data for fetching a ping? Since we've
> > decided to save the pings in dated directories, are we encoding the date in
> > the ping ID somehow for easy lookup?
> 
> We had a discussion on thursday about bug 1137252 and related issues: 
> We decided to store pings as <YYYY-MM>/<TIMESTAMP>.<UUID>.<type>.json, which
> covers our different concerns.

That doesn't really answer my question. In the .getTelemetryPingList API, will the UUID that is returned be just "<UUID>" or will it be a longer string, perhaps "YYYY-MM-UUID"?

If it's just the UUID, how is the implementation going to look up the ping without re-scanning all the dated directories?
Flags: needinfo?(gfritzsche)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> > For the request, i think it makes more sense to allow requesting an array of
> > UUIDs - it's basically the same work and allows for much less round-trips
> > (given that i expect the remote content to always request N pings anyway).
> 
> Why is the number of round-trips a concern? Assuming you agree with the
> MozSelfSupport API below, why wouldn't we make the message-based API an
> exact reflection of that? I suggested having each one be separate so that
> you could represent each one as a separate promise.

I'm concerned it might be hacky or fragile to make the message-based API reflect the promise-based API directly (currently we just send messages to the page without trying to use any functions or objects in its scope).
We can have promise-based wrappers in the remote content without the above risk.
Re batching, all i'm suggesting is batching up the requests - this seems a trivial change if we would switch APIs. Sending an event for each single ID if we might request a few hundred seems not ideal.
But maybe we think about the expected use of the API differently?

> > > Also, is the ping ID sufficient data for fetching a ping? Since we've
> > > decided to save the pings in dated directories, are we encoding the date in
> > > the ping ID somehow for easy lookup?
> > 
> > We had a discussion on thursday about bug 1137252 and related issues: 
> > We decided to store pings as <YYYY-MM>/<TIMESTAMP>.<UUID>.<type>.json, which
> > covers our different concerns.
> 
> That doesn't really answer my question. In the .getTelemetryPingList API,
> will the UUID that is returned be just "<UUID>" or will it be a longer
> string, perhaps "YYYY-MM-UUID"?

Only the UUID.

> If it's just the UUID, how is the implementation going to look up the ping
> without re-scanning all the dated directories?

We will be able to look the pings up by UUID in a list we keep internally. I don't think we should leak the naming/pathing implementation details out of the Telemetry API.
Flags: needinfo?(gfritzsche) → needinfo?(benjamin)
Attachment #8588550 - Flags: feedback?(benjamin)
> I'm concerned it might be hacky or fragile to make the message-based API
> reflect the promise-based API directly (currently we just send messages to
> the page without trying to use any functions or objects in its scope).
> We can have promise-based wrappers in the remote content without the above
> risk.

Yes, the remote content should have wrappers that match exactly the MozSelfSupport API, so that in the future when the remote content gets direct access to MozSelfSupport we don't have to rewrite it again. That means that the message passing is just a temporary conduit, and should match the actual API 

That seems to mean to me that we should make the message API at least match the SelfSupport API directly (that is, nonbatched).
Flags: needinfo?(benjamin)
Attached file API sketch (obsolete) —
Ok, i don't quite see why that should be a big issue (trivial to wrap either way), but don't care too much either.
Let's move forward here.
Attachment #8588550 - Attachment is obsolete: true
Attachment #8591626 - Flags: feedback?(benjamin)
Comment on attachment 8591626 [details]
API sketch

I'd really like to review the .webidl changes. The message-passing API should just be an implementation detail.
Attachment #8591626 - Flags: feedback?(benjamin)
Attached patch Update MozSelfSupport (obsolete) — Splinter Review
Attachment #8591784 - Flags: feedback?(benjamin)
Attachment #8591626 - Attachment is obsolete: true
Comment on attachment 8591784 [details] [diff] [review]
Update MozSelfSupport

What will APIVersion return? Do you think it's necessary?

getCurrentTelemetrySessionPing is used for what? to power about:telemetry? I'd leave it out unless there's a really good reason we need it.
Attachment #8591784 - Flags: feedback?(benjamin) → feedback+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> Comment on attachment 8591784 [details] [diff] [review]
> Update MozSelfSupport
> 
> What will APIVersion return? Do you think it's necessary?

On second thought, it's not necessary at this point. Let's leave it and add it when needed.

> getCurrentTelemetrySessionPing is used for what? to power about:telemetry?
> I'd leave it out unless there's a really good reason we need it.

Looking at about:healthreport we need to get the current session data too to support the data shown (e.g. inactive addons).
I'm not entirely happy with the test changes, but i don't really want to end up completely rewriting those tests now.
Attachment #8607641 - Flags: review?(benjamin)
Comment on attachment 8607641 [details] [diff] [review]
Update the SelfSupport API for the unification changes

getCurrentTelemetrySessionPing is actually the current subsession, right? Because prior subsessions will be in the archived ping list. I suggest renaming this for clarity.
Attachment #8607641 - Flags: review?(benjamin) → review+
Attachment #8591784 - Attachment is obsolete: true
Comment on attachment 8607641 [details] [diff] [review]
Update the SelfSupport API for the unification changes

It is not quite clear why so much 'object' use here, especially 
getTelemetryPingList could perhaps use some dictionary.
But fine, this is ChromeOnly interface.
Attachment #8607641 - Flags: review+
Attachment #8607641 - Attachment is obsolete: true
Attachment #8612941 - Flags: review+
Keywords: checkin-needed
Is there a Try link handy for this? :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c3e5504409df
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Whiteboard: [uplift]
Comment on attachment 8612941 [details] [diff] [review]
Update the SelfSupport API for the unification changes

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8612941 - Flags: approval-mozilla-aurora?
Comment on attachment 8612941 [details] [diff] [review]
Update the SelfSupport API for the unification changes

Unified telemetry is an important new feature. It is blocking some other projects. Taking it.
Attachment #8612941 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.