764190 - PLACES_EXPIRATION_STEPS_TO_CLEAN telemetry might be misleading

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #763359 +++

Please see my investigation in bug 763351 and Bug 763359.

A linear histogram with range (1, n) specified to have n buckets will actually have (n+1) buckets with a range slightly less than 1, instead of n buckets with range of 1. Consequently, sometimes two distinct values will get mapped into the same bucket. For example, in bug 763351, the values 2 and 3 both got mapped into the 2 bucket.

Since this probe seems to want to measure an integral number of steps, it seems like a good idea to change the probe to avoid this issue.

Comment 1

[Devdatta Akhawe [:devd]]

How is current code handling this issue? Just creating n+1 buckets?

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to dev from comment #1)
> How is current code handling this issue? Just creating n+1 buckets?

Yes, if you are storing counts or other integers in buckets that aren't enums.

For enums, (e.g. SSL cipher suites), you can use HISTOGRAM_ENUMERATED_VALUES.

Comment 3

[Marco Bonardo [:mak]]

likely we may just fix the number of buckets here, the thing we care most is that this measure stays low (under 5), so the actual value has less importance. We will just get more precice infos once the fix gets out in the wild.

Comment 4

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

Not sure I understand the tracking 14+ flags here - this isn't something that's crucial to fix ASAP, is it?

Comment 5

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> Not sure I understand the tracking 14+ flags here - this isn't something
> that's crucial to fix ASAP, is it?

nope, as I said in comment 3, we may just fix the measure and get new results, but not high priority.

Comment 6

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

OK, un-tracking for 14.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

taking off tracking for 15/16 as well since this isn't a crucial fix.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: patch

Just use HISTOGRAM_ENUMERATED_VALUES.  It doesn't sound like we're terribly attached to the exact number of buckets here.

Comment 9

[(dormant account)]

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

looks reasonable

Comment 10

[(dormant account)]

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

however mak is the only who needs to r+ this

Comment 11

[Marco Bonardo [:mak]]

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

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

thanks

Comment 12

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e742d57ce7

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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