Add basic app info section to top level of FHR payload

RESOLVED FIXED in Firefox 21

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
8 months ago

People

(Reporter: gps, Assigned: rnewman)

Tracking

unspecified
Firefox 22

Firefox Tracking Flags

(firefox20 unaffected, firefox21 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
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.
Won't this just move the failure to fetch appinfo somewhere else (and potentially trash the whole payload)?
Reporter

Comment 3

6 years ago
(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.
Can we just do that within the appinfo provider then?
Reporter

Comment 5

6 years ago
(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.
Assignee

Updated

6 years ago
Priority: -- → P1
Assignee

Updated

6 years ago
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee

Updated

6 years ago
Summary: Consider moving app info section into top level of payload → Add basic app info section to top level of FHR payload
Assignee

Comment 6

6 years ago
New payload:

{
  "version": 2,
  "thisPingDate": "2013-03-14",
  "geckoAppInfo": {
    "_v": 1,
    "vendor": "Mozilla",
    "name": "xpcshell",
    "id": "xpcshell@tests.mozilla.org",
    "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!
Attachment #724808 - Flags: review?(gps)
(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.
Reporter

Comment 8

6 years ago
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.
Assignee

Comment 9

6 years ago
(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.
Reporter

Comment 10

6 years ago
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.
Attachment #724808 - Flags: review?(gps) → review+
Assignee

Comment 12

6 years ago
https://hg.mozilla.org/mozilla-central/rev/9db3cfddc9be
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla22
Reporter

Comment 13

6 years ago
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.
Attachment #724808 - Flags: approval-mozilla-aurora?
Attachment #724808 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22

Updated

8 months ago
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.