1347302 - add animated image specific probes for several imagelib telemetry probes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[Timothy Nikkel (:tnikkel)]

To verify that discarding of animated images is performing as expected we need a bit more telemetry data. The current probes mix animated with non-animated. We expect non-animated numbers to stay the same. So we won't be able to really tell how the numbers are changing for animated images.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8847291 [details] [diff] [review]
patch

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

::: toolkit/components/telemetry/histogram-whitelists.json
@@ +363,5 @@
>      "HTTP_TRANSACTION_USE_ALTSVC",
>      "HTTP_TRANSACTION_USE_ALTSVC_OE",
>      "IMAGE_DECODE_CHUNKS",
>      "IMAGE_DECODE_COUNT",
> +    "IMAGE_ANIMATED_DECODE_COUNT",

You should not add any new entries to the whitelist.
Please just fill out the required fields in Histograms.json.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Are there any other specific questions for me?

Note that you will still need data review after technical review:
https://wiki.mozilla.org/Firefox/Data_Collection

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Removed the whilelist bits.

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Georg Fritzsche [:gfritzsche] [at workweek] from comment #2)
> You should not add any new entries to the whitelist.
> Please just fill out the required fields in Histograms.json.

Okay, I don't know what the whitelist is for, I just cargo-culted the existing probes.

(In reply to Georg Fritzsche [:gfritzsche] [at workweek] from comment #3)
> Are there any other specific questions for me?

I don't know, is there anything else I need to do?

> Note that you will still need data review after technical review:
> https://wiki.mozilla.org/Firefox/Data_Collection

I asked bsmedberg for feedback as that page says.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8847985 [details] [diff] [review]
patch v2

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

::: toolkit/components/telemetry/Histograms.json
@@ +1455,5 @@
> +    "low": 50,
> +    "high": 50000000,
> +    "n_buckets": 100,
> +    "description": "Time from starting a decode of an animated image to it showing up on the screen (us)"
> +  },

Maybe it has changed, but without the whitelist bits, I think you need to specify the bug_numbers and alert_emails fields (see my removal of a probe in bug 1332005). The alert_email at least will cause an email to be sent when the probe is near/at expiry as a reminder which is handy.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #6)
> Maybe it has changed, but without the whitelist bits, I think you need to
> specify the bug_numbers and alert_emails fields (see my removal of a probe
> in bug 1332005). The alert_email at least will cause an email to be sent
> when the probe is near/at expiry as a reminder which is handy.

Yeah, this appears to be enforced by the build system. I got build errors until I fixed this. Now I know what the whitelist is for.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7ac56cdb2e
Add animated image specific probes for several imagelib telemetry probes. r=aosmond f=bsmedberg

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea7ac56cdb2e