1150491 - Update remote about:healthreport content to unification changes

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Web: Health Report, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

The about:healthreport content script comes from server-side, the source is here:
https://github.com/mozilla/fhr-jelly/tree/master

We need to update this to work from the updated API from bug 1150115.

Also, we need to address versioning so we can serve content that works with both the old and the new API.
See bug 1150115, comment 2 ff.

Comment 1

[Georg Fritzsche [:gfritzsche]]

This is unblocked now - Benjamin do you know anyone who could tackle this?

Comment 2

[Benjamin Smedberg]

Suggestion for implementation: Keep the current version exactly as is, and point the client at a new URL e.g. https://fhr.cdn.mozilla.net/v4/%LOCALE%/

Comment 3

[Georg Fritzsche [:gfritzsche]]

The principal mechanics of the integration for this haven't changed.
What changed is that the message "RequestCurrentPayload" is not available anymore.
Instead we introduced the following messages:
* RequestTelemetryPingList
* RequestTelemetryPingData
* RequestCurrentEnvironment
* RequestCurrentPingData

The relevant data for addons and current startup time etc. can be taken from "RequestCurrentPingData", which is responded to with "telemetry-current-ping-data", which has the current ping data as its payload:
* ping.payload is per (startup times etc.): https://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/telemetry/main-ping.html
* ping.environment is per (addon data is in here): https://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/telemetry/environment.html

For the raw data, we decided to just point to about:telemetry. That already provides facilities to inspect the ping data in detail.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Suggestion for implementation: Keep the current version exactly as is, and
> point the client at a new URL e.g. https://fhr.cdn.mozilla.net/v4/%LOCALE%/

That looks fine to me, but i'd like server-side feedback for this.
Is this reasonable or would something else work better?

Comment 5

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] [away until july 22] from comment #4)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> > Suggestion for implementation: Keep the current version exactly as is, and
> > point the client at a new URL e.g. https://fhr.cdn.mozilla.net/v4/%LOCALE%/
> 
> That looks fine to me, but i'd like server-side feedback for this.
> Is this reasonable or would something else work better?

Comment 6

[Katie Parlante]

I went and looked at the code (and had a brief chat with lonnen), and recommend https://fhr.cdn.mozilla.net/%LOCALE%/v4/ instead. fhr-jelly already has a second version (mobile) and its url is https://fhr.cdn.mozilla.net/%LOCALE%/mobile/. This way we don't have to host two different versions of fhr-jelly or change other hosting details. We can create some new resources (new template for v4, new analog to data.js at the least), and then tweak generate.py to create a v4 directory with the desired resources (new & old).

lonnen & laura, let me know if I'm off track.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Ok, great - when can we duplicate the current report page under the new URL?
I can't land bug 1180673 before we do that. Should i maybe file a separate bug about that?

Comment 8

[Lonnen :lonnen]

Katie -- did https://github.com/mozilla/fhr-jelly/pull/171 resolve this?

Comment 9

[Georg Fritzsche [:gfritzsche]]

That resolved the request from comment 7, thanks.
There is still the original work left here to update the content to use the Telemetry instead of the FHR data.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Katie merged in the updates we have for v4 here: https://github.com/kparlante/fhr-jelly/

I've tested this locally and it looks fine here.

I've checked through the remaining localization questions here:
* channel name:
  https://github.com/kparlante/fhr-jelly/blob/master/js/data_v4.js#L54
  I think we can skip translating this, i don't see us translating channel names on e.g. mozilla.org:
  https://www.mozilla.org/zh-TW/firefox/channel/#developer
* different simple text content depending on flag:
  https://github.com/kparlante/fhr-jelly/blob/master/js/data_v4.js#L56
  How about we just use different <span> elements in the template and show the one we want per CSS?
  https://github.com/kparlante/fhr-jelly/blob/master/templates/v4.html#L75
  Then this can just go through the localization process for the template fhr-jelly already has.
  Alternatively we could feed some localizable small json or js file through the template engine?
* time span formatting:
  https://github.com/kparlante/fhr-jelly/blob/master/js/data_v4.js#L164
  This looks like additional work (pulling in some library for locale-sensitive time span formatting?).
  Can we defer this and just do what v2 did or similar (timespan value in minutes + ' min')?
  This doesn't break anything over the v2 version we have now.

Comment 11

[Katie Parlante]

(In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment #10)
> I've checked through the remaining localization questions here:
> * channel name:
>   https://github.com/kparlante/fhr-jelly/blob/master/js/data_v4.js#L54
>   I think we can skip translating this, i don't see us translating channel
> names on e.g. mozilla.org:
>   https://www.mozilla.org/zh-TW/firefox/channel/#developer
Sounds good to me.

> * different simple text content depending on flag:
>   https://github.com/kparlante/fhr-jelly/blob/master/js/data_v4.js#L56
>   How about we just use different <span> elements in the template and show
> the one we want per CSS?
>   https://github.com/kparlante/fhr-jelly/blob/master/templates/v4.html#L75
>   Then this can just go through the localization process for the template
> fhr-jelly already has.
>   Alternatively we could feed some localizable small json or js file through
> the template engine?
Yeah, moving the localizable strings to the template seems like the way to go.

> * time span formatting:
>   https://github.com/kparlante/fhr-jelly/blob/master/js/data_v4.js#L164
>   This looks like additional work (pulling in some library for
> locale-sensitive time span formatting?).
>   Can we defer this and just do what v2 did or similar (timespan value in
> minutes + ' min')?
>   This doesn't break anything over the v2 version we have now.
I'm sympathetic to the "just do what v2 did" argument, though v2 version looks like a localization bug to me. Perhaps this file gets updated with "minutes", "hours" and "days": https://github.com/mozilla/fhr-jelly/blob/master/js/locale/date_format.json

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to Katie Parlante from comment #11)
> > * time span formatting:
> >   https://github.com/kparlante/fhr-jelly/blob/master/js/data_v4.js#L164
> >   This looks like additional work (pulling in some library for
> > locale-sensitive time span formatting?).
> >   Can we defer this and just do what v2 did or similar (timespan value in
> > minutes + ' min')?
> >   This doesn't break anything over the v2 version we have now.
> I'm sympathetic to the "just do what v2 did" argument, though v2 version
> looks like a localization bug to me. Perhaps this file gets updated with
> "minutes", "hours" and "days":
> https://github.com/mozilla/fhr-jelly/blob/master/js/locale/date_format.json

I think its not as simple as that: we'd need proper locale-sensitive formatting to actually fix this (number formatting + is the unit a suffix or prefix or ...?).

Lets make it a follow-up and not block on this?

Comment 13

[Georg Fritzsche [:gfritzsche]]

I pushed a straight-forward approach here: https://github.com/georgf/fhr-jelly

Katie, how about that? Who could review and where would i push to?

Comment 14

[Katie Parlante]

(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> (In reply to Katie Parlante from comment #11)
> > > * time span formatting:
> > >   https://github.com/kparlante/fhr-jelly/blob/master/js/data_v4.js#L164
> > >   This looks like additional work (pulling in some library for
> > > locale-sensitive time span formatting?).
> > >   Can we defer this and just do what v2 did or similar (timespan value in
> > > minutes + ' min')?
> > >   This doesn't break anything over the v2 version we have now.
> > I'm sympathetic to the "just do what v2 did" argument, though v2 version
> > looks like a localization bug to me. Perhaps this file gets updated with
> > "minutes", "hours" and "days":
> > https://github.com/mozilla/fhr-jelly/blob/master/js/locale/date_format.json
> 
> I think its not as simple as that: we'd need proper locale-sensitive
> formatting to actually fix this (number formatting + is the unit a suffix or
> prefix or ...?).
> 
> Lets make it a follow-up and not block on this?

FWIW, looks like moment.js is a good library for this: http://momentjs.com/

Comment 15

[Katie Parlante]

(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> I pushed a straight-forward approach here:
> https://github.com/georgf/fhr-jelly
> 
> Katie, how about that? Who could review and where would i push to?

Looks good to me. You could create a PR for mozilla/fhr-jelly and and ask lonnen to review.

The one missing piece is unit tests. You can run the existing tests with grunt (http://gruntjs.com/getting-started). To add tests, I think we'll need to have v4 versions of sample pings & unit tests:
- https://github.com/mozilla/fhr-jelly/tree/master/tests/json
- https://github.com/mozilla/fhr-jelly/blob/master/tests/unit.html
- https://github.com/mozilla/fhr-jelly/blob/master/tests/lib/unit.js

Comment 16

[Georg Fritzsche [:gfritzsche]]

I'll take this over at this point then.
This is the last real shipping blocker besides data quality, so we should get this done.

Comment 17

[Katie Parlante]

Thanks Georg

Comment 18

[Thomas Huelbert]

whiteboard tag for sprint tracking

Comment 19

[Georg Fritzsche [:gfritzsche]]

So, i fixed some bugs and am trying to add unit tests here (other than tests this should be done):
https://github.com/georgf/fhr-jelly

But i can't get any useful output nor failure info from the test i added so far:
https://github.com/georgf/fhr-jelly/commit/a64e2c453a159654e29fbd5ab0c820ad4bbef843

Anyone have any idea? Run all the tasks e.g. with |grunt -v|.
Adding e.g. some simple |ok(false, "foo bar");| to unit_v4.js doesn't produce any useful output, only something like:
At other times it just randomly passes:
> Testing http://localhost:9000/tests/unit.html
...
> Testing http://localhost:9000/tests/unit_v4.html
>
> >> 7 assertions passed (122ms)
> 
> Done, without errors.

At other times i get unhelpful output like:
...
> Testing http://localhost:9000/tests/unit_v4.html
>
> >> PhantomJS timed out, possibly due to a missing QUnit start() call.
> Warning: 1/8 assertions failed (108ms) Use --force to continue.
>
> Aborted due to warnings.

Comment 20

[Lonnen :lonnen]

I'm none too clueful about Grunt, sorry. Schalk may have some more insight.

Comment 21

[Georg Fritzsche [:gfritzsche]]

In the mean-time we have an actionable short-term plan and will look into more viable alternatives later.
Dropping the needinfo for now.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Pull request opened: https://github.com/mozilla/fhr-jelly/pull/172

After review we need to start the l10n process, then we should be good to go.

Comment 23

[Georg Fritzsche [:gfritzsche]]

This is reviewed and on staging, pending l10n and QA for push to production.