924651 - BackgroundPageThumbs should log crashes as a telemetry completion reason

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

I asked in #telemetry whether it's OK to increase a histogram's n_values and haven't gotten a reply, but [1, 2] changed some histograms from using n_buckets to n_values, and [3] changed the numbers of n_buckets.  In this case, we're only adding more values at the end, so it would seem OK...  I increased the number to 10 to avoid having to increase it again, although I don't foresee any new possible values.

[1] http://hg.mozilla.org/mozilla-central/diff/d77ea5e2c469/toolkit/components/telemetry/Histograms.json
[2] http://hg.mozilla.org/mozilla-central/diff/e36689713d6c/toolkit/components/telemetry/Histograms.json
[3] http://hg.mozilla.org/mozilla-central/diff/d800ecb735c9/toolkit/components/telemetry/Histograms.json

Comment 1

[Drew Willcoxon :adw]

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

Nathan, this patch increases the n_values value of an enumerated histogram.  It doesn't change the meaning of any of the current values in the enumeration, but it does add new values with new meanings.  Is that OK, or do we need to make a new histogram?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

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

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

The thumbnail code looks fine, but:

> I increased the number to 10 to avoid having to increase it again,
> although I don't foresee any new possible values.

ISTM that it's either fine to do it now (so would be OK to do again), or it's *not* OK to do now (in which case we need to do something special, and the extra unused overhead might make sense).  I've no idea what n_values means in practice (ie, why it must be specified and what the implications of that are), so r=me on the thumbnail code only, and I'm changing nfroyd's feedback request to a review request on Histograms.json.

Comment 3

[Nathan Froyd [:froydnj]]

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

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

Adding extra values that you're not using currently is fine, just so long as you're not adding a ton of new values.  What you have is sensible.

I have a feeling that the server side might get a little upset at trying to merge FX_THUMBNAILS_BG_CAPTURE_DONE_REASON histograms when the parameters of the histogram change suddenly.  I'd recommend tacking a "2" onto the histogram name.  r=me with that.

Comment 4

[Drew Willcoxon :adw]

Thanks, Nathan, landed with comments addressed:

https://hg.mozilla.org/integration/fx-team/rev/15bab6ba175d

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15bab6ba175d

Comment 6

[Drew Willcoxon :adw]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 910563, but this isn't a regression.
User impact if declined: This will allow us to quickly gather more telemetry data for a particular telemetry probe by including Aurora users in addition to Nightly users.
Testing completed (on m-c, etc.): No particular testing needed for this patch.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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

This isn't the patch that landed, right? It's generally a good idea to upload the actual patch you intend to land, for approval. But a=me to land the equivalent of https://hg.mozilla.org/mozilla-central/rev/15bab6ba175d on Aurora.

Comment 8

[Drew Willcoxon :adw]

Attachment: Aurora (26) patch

Yeah, I realized that after I made the request, and I was starting to upload the final patch, which also applies cleanly to Aurora, and cancel the old request when you approved it. :-)

Just to do it by the book, would you mind approving this one and canceling the previous one?

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 910563, but this isn't a regression.
User impact if declined: This will allow us to quickly gather more telemetry data for a particular telemetry probe by including Aurora users in addition to Nightly users.
Testing completed (on m-c, etc.): No particular testing needed for this patch.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 9

[Drew Willcoxon :adw]

https://hg.mozilla.org/releases/mozilla-aurora/rev/829a06f8d058