1172688 - Add telemetry for when gethash calls timeout

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[François Marier [:francois]]

It would be useful to know how often these completions timeout when trying to reach Google's servers.

Comment 1

[François Marier [:francois]]

This is probably going to be similar to bug 1150921.

Comment 2

[Dimi Lee [ooo 16.04 - 21.04］]

I would like to work on this bug.

Comment 3

[Dimi Lee [ooo 16.04 - 21.04］]

Hi François,
Do we measure this by how many success gethash request with a timeout or the time between each timeout ?

Comment 4

[François Marier [:francois]]

I don't think we have a need for the actual time it took. We just want to make sure that requests don't time out very often.

So I would suggest: # timeouts / (# timeouts + # completed requests)

which means we also need to add telemetry for the requests that succeeded.

Comment 5

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Add telemetry for gethash

Comment 6

[François Marier [:francois]]

Comment on attachment 8732696 [details] [diff] [review]
Add telemetry for gethash

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

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
@@ +553,5 @@
>      // Notify the RequestBackoff once a response is received.
>      this._completer.finishRequest(this.gethashUrl, httpStatus);
>  
>      if (success) {
> +      Services.telemetry.getHistogramById("URLCLASSIFIER_COMPLETE_TIMEOUT").add(0);

What happens when the request doesn't timeout but still fails? For example, if it returns a 503 instead of a 200.

I would say that as far as the telemetry goes, we should consider that a non-timeout event and mark it as such. We're only interested in seeing how often the gethash endpoint fails to answer at all.

Comment 7

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Add telemetry for gethash v2

Modification of this patch:
- Record all gethash requests in telemetry instead of just record success requests

Comment 8

[François Marier [:francois]]

Comment on attachment 8733220 [details] [diff] [review]
Add telemetry for gethash v2

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

That looks fine to me. I think Benjamin needs to review all new telemetry probes.

Comment 9

[Benjamin Smedberg]

Comment on attachment 8733220 [details] [diff] [review]
Add telemetry for gethash v2

If this is an exploratory measurement, it should expire (typically in 6 months).

If this is an operational alerting metric, I think we should be collecting from the release population, and you should talk a bit about how you plan on monitoring this with either daily or realtime alerting.

Comment 10

[François Marier [:francois]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> If this is an exploratory measurement, it should expire (typically in 6
> months).
> 
> If this is an operational alerting metric, I think we should be collecting
> from the release population, and you should talk a bit about how you plan on
> monitoring this with either daily or realtime alerting.

The question we're trying to answer is: Is the 10s timeout reasonable and (especially) does this happen often in practice?

Is that what you'd consider an exploratory measurement? If so, can we run it for 1 year instead of just 6 months?

Comment 11

[Benjamin Smedberg]

Please run it for 6 months first and if you still need more data you can extend it.

You could instead add a metric that measures the total time a gethash call takes, which would give you a distribution and not just a boolean.

Comment 12

[François Marier [:francois]]

Dimi, I think we should go with the 6 months that Benjamin suggested.

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1237198&attachment=8735634 suggests that all we need to do is add "expires_in_version" with whatever release will come 6 months after Fx 48.

Comment 13

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: MozReview Request: Bug 1172688 - Add telemetry for when gethash calls timeout. r=francois, bsmedberg

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

Comment 14

[François Marier [:francois]]

Comment on attachment 8737773 [details]
MozReview Request: Bug 1172688 - Add telemetry for when gethash calls timeout. r=francois, bsmedberg

https://reviewboard.mozilla.org/r/44105/#review40815

Comment 15

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Dimi Lee[:dimi][:dlee] from comment #13)
> Created attachment 8737773 [details]
> MozReview Request: Bug 1172688 - Add telemetry for when gethash calls
> timeout. r?francois, bsmedberg
> 
> Review commit: https://reviewboard.mozilla.org/r/44105/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/44105/

Forget to say, based on comment 4, i still use boolean for now instead of metric suggested in comment 11

Comment 16

[François Marier [:francois]]

I think the boolean is fine for this initial exploration.

Comment 17

[Benjamin Smedberg]

Comment on attachment 8737773 [details]
MozReview Request: Bug 1172688 - Add telemetry for when gethash calls timeout. r=francois, bsmedberg

https://reviewboard.mozilla.org/r/44105/#review41015

data-review=me with that change or something equivalent

::: toolkit/components/telemetry/Histograms.json:3364
(Diff revision 1)
> +  "URLCLASSIFIER_COMPLETE_TIMEOUT": {
> +    "alert_emails": ["[e-mail]", "[e-mail]"],
> +    "expires_in_version": "52",
> +    "kind": "boolean",
> +    "bug_numbers": [1172688],
> +    "description": "True if gethash lookup timeout"

Suggest more detail: "this metric is recorded every time a gethash lookup is performed. `true` is recorded if the lookup times out."

Comment 18

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8737773 [details]
MozReview Request: Bug 1172688 - Add telemetry for when gethash calls timeout. r=francois, bsmedberg

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/51688b3611d4

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/51688b3611d4