845431 - Send errors in FHR report

Status: RESOLVED FIXED

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

Description

[Gregory Szorc [:gps]]

FHR currently captures some exceptional errors in collection state. However, they aren't consumed by anything (just reported on in logging which likely goes unnoticed).

The original intent of these errors was to include them in the FHR payload. Let's actually hook that up!

It's worth noting that any submitted errors must not contain PII or other sensitive info without explicit user consent. When we implemented error capture, I believe we made sure that all error strings are static or mostly static as to minimize this possibility. We should verify that.

Long term, I'd like to see some kind of report out of Metrics summarizing all the errors so we can fix bugs as soon as possible.

Mark: Do you need any notice before we push out a revised payload format?

Comment 1

[Gregory Szorc [:gps]]

Here are the current errors we collect:

---

When invoking a collect* function on an individual provider:
      try {
        collectPromise = provider[fnProperty].call(provider);
      } catch (ex) {
        this._log.warn("Exception when calling " + provider.name + "." +
                       fnProperty + ": " + CommonUtils.exceptionStr(ex));
        this.providerErrors.get(provider.name).push(ex);
        continue;
      }

While we include full exception details, we are in a collect function. Presumably any user data creeping into the exception string was being collected and is thus covered by FHR's already consented to upload policy. The stack should also be clean as collection is driven by a timer installed by FHR.

---

When waiting on a provider to finish collecting:

        try {
          yield promise;
          this._log.debug("Provider collected successfully: " + name);
        } catch (ex) {
          this._log.warn("Provider failed to collect: " + name + ": " +
                         CommonUtils.exceptionStr(ex));
          this.providerErrors.get(name).push(ex);
        }

I believe the same explanation as the above applies.

---

When obtaining a serializer for a measurement:

        try {
          // The measurement is responsible for returning a serializer which
          // is aware of the measurement version.
          serializer = measurement.serializer(measurement.SERIALIZE_JSON);
        } catch (ex) {
          this._log.warn("Error obtaining serializer for measurement: " + name +
                         ": " + CommonUtils.exceptionStr(ex));
          errors.push("Could not obtain serializer: " + name);
          continue;
        }

The error string simply identifies the measurement name. No concerns here.

---

When obtaining data for a measurement:

        try {
          data = yield measurement.getValues();
        } catch (ex) {
          this._log.warn("Error obtaining data for measurement: " +
                         name + ": " + CommonUtils.exceptionStr(ex));
          errors.push("Could not obtain data: " + name);
          continue;
        }

The string does not leak anything private. I would argue we should include a stack trace here. Again, the reasoning from above applies: any user data in the exception would have to be stored in the database. And, everything in the database is fit for submission to the server.

---

When serializing "singular" data:

          try {
            o.data.last[name] = serializer.singular(data.singular);
          } catch (ex) {
            this._log.warn("Error serializing data: " + CommonUtils.exceptionStr(ex));
            errors.push("Error serializing singular: " + name);
            continue;
          }

Same as above.

---

When serializing per-day data:

          try {
            let serialized = serializer.daily(dataDays.getDay(date));
            if (!serialized) {
              continue;
            }

            if (!(dateFormatted in outputDataDays)) {
              outputDataDays[dateFormatted] = {};
            }

            outputDataDays[dateFormatted][name] = serialized;
          } catch (ex) {
            this._log.warn("Error populating data for day: " +
                           CommonUtils.exceptionStr(ex));
            errors.push("Could not serialize day: " + name +
                        " ( " + dateFormatted + ")");
            continue;
          }

Same as above.

---

And lo while I was here I noticed that we add this latter set of errors to the payload currently! We should ask Metrics if we're seeing anything!

This means this bug would be about incorporating collection errors into the payload and possibly including stacks for the existing errors.

Comment 2

[Mark Reid [:mreid]]

Generally speaking, it will not break anything to add new things to the payload (though as you say, 'errors' is actually not new).

We /do/ need advance notice of format/semantic changes so that we can adjust any analysis and ETL code accordingly.

Comment 3

[Mark Reid [:mreid]]

Attachment: A listing of FHR payloads with 'errors' by date

I did a quick check to see what we were getting in the 'errors' field - looks like a grand total of 28 payloads (attached).

Comment 4

[Gregory Szorc [:gps]]

Attachment: Send more errors in FHR payload, v1

Still not sending stacks. But, it's better than it was before.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 719208 [details] [diff] [review]
Send more errors in FHR payload, v1

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

::: services/healthreport/healthreporter.jsm
@@ +565,5 @@
>      }.bind(this)).then(onFinished, onFinished);
>    },
>  
>    /**
> +   * Record an exception for reporting in the payload.

Record in the payload an exception to be reported.

@@ +792,5 @@
>        }
>      }
>  
> +    if (this._errors.length) {
> +      o.errors = this._errors;

Suggestion: truncate this._errors to some sane value.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0639d2dca7a6

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/0639d2dca7a6

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 719208 [details] [diff] [review]
Send more errors in FHR payload, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  None.

User impact if declined: 
  Makes it more difficult for us to detect errors in FHR upstream.

Testing completed (on m-c, etc.): 
  Baking happily on Nightly.

Risk to taking this patch (and alternatives if risky): 
  Very low.

String or UUID changes made by this patch: 
  None.

Comment 9

[Richard Newman [:rnewman]]

Let's make sure privacy are OK with this.

Comment 10

[Richard Newman [:rnewman]]

To clarify: we want to make sure privacy are OK with this *prior to it reaching a Beta audience*.

Flagging Alex to opine on this at some point. Let me know if you need more details.

Comment 11

[bhavana bajaj [:bajaj]]

Comment on attachment 719208 [details] [diff] [review]
Send more errors in FHR payload, v1

Approving the low risk uplift for aurora as the privacy review is needed only for beta/release.

Comment 12

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb018f4077d4

Comment 13

[Gregory Szorc [:gps]]

Privacy review is being tracked in bug 852308.

Comment 14

[u279076]

How can I check to see if this is working now? I don't see any "errors" in my Show Details page.

Comment 15

[Gregory Szorc [:gps]]

We have test coverage of this and server-side data reveals errors are being sent.