BackgroundPageThumbs should log crashes as a telemetry completion reason

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed)

Details

Attachments

(2 attachments)

Posted patch patchSplinter Review
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
Attachment #814591 - Flags: review?(mhammond)
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?
Attachment #814591 - Flags: feedback?(nfroyd)
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.
Attachment #814591 - Flags: review?(nfroyd)
Attachment #814591 - Flags: review?(mhammond)
Attachment #814591 - Flags: review+
Attachment #814591 - Flags: feedback?(nfroyd)
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.
Attachment #814591 - Flags: review?(nfroyd) → review+
Thanks, Nathan, landed with comments addressed:

https://hg.mozilla.org/integration/fx-team/rev/15bab6ba175d
https://hg.mozilla.org/mozilla-central/rev/15bab6ba175d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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
Attachment #814591 - Flags: approval-mozilla-aurora?
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.
Attachment #814591 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #815591 - Flags: approval-mozilla-aurora?
Attachment #815591 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #814591 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.