1336376 - Expand the range of values for URLCLASSIFIER_LOOKUP_TIME

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

Right now, URLCLASSIFIER_LOOKUP_TIME is defined like this:

    "kind": "exponential",
    "high": 500,
    "n_buckets": 10,

which unfortunately hides how bad lookup time can be by putting everything over 500 ms in the same bucket.

We should bump the number of buckets and move the high to at least 5 seconds.

Comment 1

[François Marier [:francois]]

Attachment: Bug 1336376 - Expand the range of values for URLCLASSIFIER_LOOKUP_TIME.

Review commit: https://reviewboard.mozilla.org/r/109526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109526/

Comment 2

[François Marier [:francois]]

Benjamin, is there a problem with increasing the number of buckets and/or the high value of an existing probe?

https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe points out that n_values can't be increased later for technical reasons, but is silent when it comes to n_buckets. The only thing I could find in there is a recommendation that a new probe be created instead, but no explanation is given.

Comment 3

[Benjamin Smedberg]

Aggregation and t.m.o will break if you change a histogram like this without changing the name. Since you're changing the bucket boundaries anyway, the best practice is to change the name to URLCLASSIFIER_LOOKUP_TIME_2 and so forth.

Comment 4

[Benjamin Smedberg]

Comment on attachment 8833273 [details]
Bug 1336376 - Expand the range of values for URLCLASSIFIER_LOOKUP_TIME.

https://reviewboard.mozilla.org/r/109526/#review110574

Comment 5

[François Marier [:francois]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #3)
> Aggregation and t.m.o will break if you change a histogram like this without
> changing the name. Since you're changing the bucket boundaries anyway, the
> best practice is to change the name to URLCLASSIFIER_LOOKUP_TIME_2 and so
> forth.

Can I delete the original probe (i.e. replace it with LOOKUP_TIME_2) or is there a technical reason why I should leave it around for a while?

Comment 6

[François Marier [:francois]]

Comment on attachment 8833273 [details]
Bug 1336376 - Expand the range of values for URLCLASSIFIER_LOOKUP_TIME.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109526/diff/1-2/

Comment 7

[Benjamin Smedberg]

I *think* we stop aggregating and displaying the old metric on t.m.o as soon as it's removed from nightly, but I'm not 100% sure of that. Ryan do you know?

Comment 8

[Benjamin Smedberg]

Comment on attachment 8833273 [details]
Bug 1336376 - Expand the range of values for URLCLASSIFIER_LOOKUP_TIME.

https://reviewboard.mozilla.org/r/109526/#review111072

Comment 9

[Ryan Harter [:harter]]

I'll let Frank answer this; He has more experience with t.m.o.

Comment 10

[Frank Bertsch [:frank]]

TLDR; You can go ahead and replace it, won't hurt a thing.

Longer story: On the backend, we aggregate anything we see, regardless of whether we expected it for a certain channel/version/build. Only on the frontend (with the aggregates service) do we try to look up what the definition is for a probe. If this lookup fails, you'll get a 404. Basically, if the probe is no longer available in histograms.json or scalars.yaml, it's not available on TMO. This has it's drawbacks (historical probes aren't available), but hasn't failed us so far.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a6b42b3d233
Expand the range of values for URLCLASSIFIER_LOOKUP_TIME. r=bsmedberg

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/autoland/rev/2ff48b77d380c9c649d2f5f1d54b4f5c5b3340ad for build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=74977218&repo=autoland

It looks like an easy fix, but I don't know enough about the whitelist to know what removing it would mean elsewhere.

Comment 13

[François Marier [:francois]]

Comment on attachment 8833273 [details]
Bug 1336376 - Expand the range of values for URLCLASSIFIER_LOOKUP_TIME.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109526/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/017a3a0d4a00
Expand the range of values for URLCLASSIFIER_LOOKUP_TIME. r=bsmedberg

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/017a3a0d4a00