Closed Bug 845127 Opened 7 years ago Closed 7 years ago

Telemetry for FHR payload wire size

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(1 file)

Let's grab telemetry for the wire size of FHR's payload. We already record uncompressed size.
This should do it!

Waiting on tree to build to test locally. I anticipate a very small delta, if any.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #718178 - Flags: review?(rnewman)
Attachment #718178 - Flags: feedback?(vdjeric)
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 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?
Attachment #718178 - Flags: review?(rnewman) → review+
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
Attachment #718178 - Flags: feedback?(vdjeric) → feedback+
(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.
(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.
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/53d3d33713c2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.