845127 - Telemetry for FHR payload wire size

Status: RESOLVED FIXED

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

Description

[Gregory Szorc [:gps]]

Let's grab telemetry for the wire size of FHR's payload. We already record uncompressed size.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Compressed payload size telemetry, v1

This should do it!

Waiting on tree to build to test locally. I anticipate a very small delta, if any.

Comment 2

[Gregory Szorc [:gps]]

Tested locally and this appears to work. Bucket sizes are 10k. Do we want to go finer? I anticipate that the incremental increase in compressed size will be a shallower curve of uncompressed due to repeated strings. I certainly wouldn't mind more granularity...

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 718178 [details] [diff] [review]
Compressed payload size telemetry, v1

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

r+ with concerns:

* What are you trying to measure: wire size, compression ratio, or both?
* Waiting for perf team input as to bucket size.

::: services/common/bagheeraclient.js
@@ +113,5 @@
>     *        JSON.stringify() for serialization.
> +   * @param options
> +   *        (object) Extra options to control behavior. Recognized properties:
> +   *
> +   *          deleteOldID -- (string) Old document ID to delete as part of

You could just simplify this to "delete".

@@ +125,4 @@
>     *
>     * @return Promise<BagheeraClientRequestResult>
>     */
> +  uploadJSON: function uploadJSON(namespace, id, payload, options={}) {

No need to name the function, mm?

::: services/healthreport/healthreporter.jsm
@@ +1141,5 @@
>        let result;
>        try {
> +        let options = {
> +          deleteOldID: this.lastSubmitID,
> +          telemetryCompressed: TELEMETRY_PAYLOAD_SIZE_COMPRESSED,

I'm not sure how valuable it is to have two separate histograms.

Might it actually be advantageous to report the compression ratio as well (or instead), if that's what you care about?

Comment 4

[Vladan Djeric (:vladan)]

Comment on attachment 718178 [details] [diff] [review]
Compressed payload size telemetry, v1

You don't really need the try-block around getHistogramById()

See https://bugzilla.mozilla.org/show_bug.cgi?id=810149#c12

Comment 5

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #3)

> ::: services/common/bagheeraclient.js
> @@ +113,5 @@
> >     *        JSON.stringify() for serialization.
> > +   * @param options
> > +   *        (object) Extra options to control behavior. Recognized properties:
> > +   *
> > +   *          deleteOldID -- (string) Old document ID to delete as part of
> 
> You could just simplify this to "delete".

I don't like using built-in keywords as property names.

> @@ +125,4 @@
> >     *
> >     * @return Promise<BagheeraClientRequestResult>
> >     */
> > +  uploadJSON: function uploadJSON(namespace, id, payload, options={}) {
> 
> No need to name the function, mm?

The whole file is like this. Want me to part 0 it?

> ::: services/healthreport/healthreporter.jsm
> @@ +1141,5 @@
> >        let result;
> >        try {
> > +        let options = {
> > +          deleteOldID: this.lastSubmitID,
> > +          telemetryCompressed: TELEMETRY_PAYLOAD_SIZE_COMPRESSED,
> 
> I'm not sure how valuable it is to have two separate histograms.

You're right: there's probably a marginal value to having both. The server obviously sees the wire size and can derive the uncompressed size.

I'm fishing here, but it could be used for correlation with other Telemetry numbers. We do save the last uploaded payload to disk uncompressed. And, if there is a large discrepancy in size, that could explain some things.

I don't think it costs too much to update a histogram and more data is better than no data, right? If nothing else, this gives us a basic visualization of wire size through the Telemetry dashboard until the FHR dashboard is ready.

> Might it actually be advantageous to report the compression ratio as well
> (or instead), if that's what you care about?

I dunno.

Comment 6

[Richard Newman [:rnewman]]

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

> > You could just simplify this to "delete".
> 
> I don't like using built-in keywords as property names.

Then "deleteID"? :D

> The whole file is like this. Want me to part 0 it?

Nah.

> > I'm not sure how valuable it is to have two separate histograms.
> 
> You're right: there's probably a marginal value to having both. The server
> obviously sees the wire size and can derive the uncompressed size.

I'm actually thinking more that the two values are directly related ("before and after"), but the two histograms are not. If, say, we're getting half our payloads slightly *increased* in size, we can't really tell that from the histograms.

So let's figure out what we're trying to measure here. Do we care about how much we're sending and how well it compresses (e.g., to decide if it's worth compressing protobufs, or to evaluate different compression mechanisms, or to find a bug in the compressor, or...), or do we care how much user bandwidth we're consuming, or both?

If we care about compression, we should be recording ratios, not two separated values.

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53d3d33713c2

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/53d3d33713c2