849353 - Add basic app info section to top level of FHR payload

Status: RESOLVED FIXED

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

Description

[Gregory Szorc [:gps]]

We should consider moving the app info section to the top level of the FHR payload.

If we implement bug 846083, we could start generating near-empty payloads: they would be payloads with just errors. While the value of the errors in those payloads would be useful, we wouldn't be able to correlate occurrences with app versions. This would make assessing the impact of these very difficult because we can't answer the question "are clients with the fix still reporting the error."

I propose we move the app info section (or at least part of it) to the top-level of the document and we *always* attempt to send some minimal details, such as the build id and channel name.

Comment 1

[Richard Newman [:rnewman]]

Fine by me. We can easily deliver static values like this, essentially templated into the response.

Just consider that on Android, two components will be involved in the upload, potentially across two different versions.

Comment 2

[Mark Reid [:mreid]]

Won't this just move the failure to fetch appinfo somewhere else (and potentially trash the whole payload)?

Comment 3

[Gregory Szorc [:gps]]

(In reply to Mark Reid [:mreid] from comment #2)
> Won't this just move the failure to fetch appinfo somewhere else (and
> potentially trash the whole payload)?

Not if it's in a try..catch block!

I suspect missing appinfo is caused by something really weird, not app info itself. If we factored out just the basic field getters and did no DB interaction, etc, I highly doubt it would ever fail.

Comment 4

[Mark Reid [:mreid]]

Can we just do that within the appinfo provider then?

Comment 5

[Gregory Szorc [:gps]]

(In reply to Mark Reid [:mreid] from comment #4)
> Can we just do that within the appinfo provider then?

We do.

https://hg.mozilla.org/mozilla-central/file/af5f267cd6a1/services/healthreport/providers.jsm#l110

That is already liberally littered with try..catch blocks. This is why the missing app info section is blowing my mind: it shouldn't be throwing errors.

Comment 6

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

New payload:

{
  "version": 2,
  "thisPingDate": "2013-03-14",
  "geckoAppInfo": {
    "_v": 1,
    "vendor": "Mozilla",
    "name": "xpcshell",
    "id": "[e-mail]",
    "version": "1",
    "appBuildID": "20121107",
    "platformVersion": "p-ver",
    "platformBuildID": "20121106",
    "os": "XPCShell",
    "xpcomabi": "noarch-spidermonkey",
    "updateChannel": "default"
  },
  "data": {
    "last": {},
    "days": {}
  }
}

Note that there's a small bit of copypasta between AppInfoProvider's field definitions and HealthReporter. I view that as better than introducing some shared location.

Note also that I elected to *duplicate* some of this data, rather than *move* the existing appInfo provider. Potentially we'll have a different or additional blob of data on Android, and we want to spot the situation of appinfo being missing!

Comment 7

[Mark Reid [:mreid]]

(In reply to Gregory Szorc [:gps] from comment #5)
> (In reply to Mark Reid [:mreid] from comment #4)
> > Can we just do that within the appinfo provider then?
> 
> We do.
> 
> https://hg.mozilla.org/mozilla-central/file/af5f267cd6a1/services/
> healthreport/providers.jsm#l110
> 
> That is already liberally littered with try..catch blocks. This is why the
> missing app info section is blowing my mind: it shouldn't be throwing errors.

Hang on, doesn't this mean we *shouldn't* move this to the top level of the payload?  At least, not until we fix the mind-blowing disappearance of the app info section?

Seems to me that this change introduces a risk (however small) of not generating the payload at all.  For example, not sure how likely it is, but if something throws "null", then the log call to 'CommonUtils.exceptionStr(ex)' will throw TypeError.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 724808 [details] [diff] [review]
Proposed patch. v1

Review of attachment 724808 [details] [diff] [review]:
-----------------------------------------------------------------

Before I grant review, I'd like to know why we're effectively copying code for generation of this object instead of refactoring the existing code in the provider to support both consumers.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #8)

> Before I grant review, I'd like to know why we're effectively copying code
> for generation of this object instead of refactoring the existing code in
> the provider to support both consumers.

• No promises. This is all 'normal' JS. If there's a problem with promises that's causing appInfo generation to fail, this will succeed.
• Ability to change the two independently; fallback metrics code can continue to use a stable top-level descriptor, even as the appInfo provider evolves to meet changing needs.
• No runtime dependency on a provider managed by category. No additional coupling.
  • It's not much code. Introducing a shared component to do this work would be a bigger diff.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 724808 [details] [diff] [review]
Proposed patch. v1

Review of attachment 724808 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not crazy about the redundancy. But, meh. There should be a minimal wire size impact due to compression.

We can follow up with de-duplication later.

Comment 11

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/9db3cfddc9be

Comment 12

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/9db3cfddc9be

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 724808 [details] [diff] [review]
Proposed patch. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR core feature
User impact if declined: Subtle bugs in FHR data collection may manifest as unusable FHR payloads.
Testing completed (on m-c, etc.): It's baked for a few days. There is testing coverage.
Risk to taking this patch (and alternatives if risky): Some FHR payloads may not contain important application metadata.
String or UUID changes made by this patch: None.

This bug is triaged as P1. This means its a beta blocker. This needs to be uplifted if FHR is to be enabled in Beta/21.

Not having this patch may render some FHRs payloads next to useless because they are lacking critical application/build information.

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1688de15d68b