Closed Bug 1311926 Opened 5 years ago Closed 5 years ago

Add telemetry to measure gethash error and gethash timeout rate for V2 and V4

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: dimi, Assigned: tnguyen)

References

Details

(Whiteboard: #sbv4-m5)

Attachments

(1 file)

As discussed in WW, we should record the gethash error rate and gethash timeout rate for v2 and v4 to get better understanding about if we could ship v4.
Priority: -- → P2
As francois suggested in Bug 1311910 Comment 5,, we should add following as keyed histogram:
`URLCLASSIFIER_COMPLETE_TIMEOUT`
`URLCLASSIFIER_COMPLETE_REMOTE_STATUS`
Whiteboard: #sbv4-m4 → #sbv4-m5
Assignee: nobody → tnguyen
Comment on attachment 8824337 [details]
Bug 1311926 - Add telemetry to measure gethash error and gethash timeout rate for V2 and V4.

https://reviewboard.mozilla.org/r/102850/#review103574

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:386
(Diff revision 1)
>      // If we haven't gotten onStopRequest, just cancel. This will call us
>      // with onStopRequest since we implement nsIStreamListener on the
>      // channel.
>      if (this._channel && this._channel.isPending()) {
>        log("cancelling request to " + this.gethashUrl + "\n");
> -      Services.telemetry.getHistogramById("URLCLASSIFIER_COMPLETE_TIMEOUT").add(1);
> +      Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_TIMEOUT").

`TIMEOUT2`

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:675
(Diff revision 1)
>        }
>      }
>      let success = Components.isSuccessCode(aStatusCode);
>      log('Received a ' + httpStatus + ' status code from the gethash server (success=' + success + ').');
>  
>      let histogram =

Maybe we should do this in a single statement, just like the next one:

    Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_REMOTE_STATUS2").
      add(this.telemetryProvider, httpStatusToBucket(httpStatus));

Right now, the two telemetry probes look different and it's a little confusing because it's not a meaningful difference.

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:676
(Diff revision 1)
>      }
>      let success = Components.isSuccessCode(aStatusCode);
>      log('Received a ' + httpStatus + ' status code from the gethash server (success=' + success + ').');
>  
>      let histogram =
> -      Services.telemetry.getHistogramById("URLCLASSIFIER_COMPLETE_REMOTE_STATUS");
> +      Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_REMOTE_STATUS");

`STATUS2`

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:678
(Diff revision 1)
>      log('Received a ' + httpStatus + ' status code from the gethash server (success=' + success + ').');
>  
>      let histogram =
> -      Services.telemetry.getHistogramById("URLCLASSIFIER_COMPLETE_REMOTE_STATUS");
> -    histogram.add(httpStatusToBucket(httpStatus));
> -    Services.telemetry.getHistogramById("URLCLASSIFIER_COMPLETE_TIMEOUT").add(0);
> +      Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_REMOTE_STATUS");
> +    histogram.add(this.telemetryProvider, httpStatusToBucket(httpStatus));
> +    Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_TIMEOUT").

`TIMEOUT2`
Attachment #8824337 - Flags: review?(francois) → review-
Thanks Francois for your review.
Comment on attachment 8824337 [details]
Bug 1311926 - Add telemetry to measure gethash error and gethash timeout rate for V2 and V4.

https://reviewboard.mozilla.org/r/102850/#review103976

Looks good. Please add the original bug numbers and land this.

::: toolkit/components/telemetry/Histograms.json:4184
(Diff revision 2)
>      "alert_emails": ["safebrowsing-telemetry@mozilla.org"],
>      "expires_in_version": "never",
>      "kind": "enumerated",
> +    "keyed": true,
>      "n_values": 16,
> -    "bug_numbers": [1150921],
> +    "bug_numbers": [1311926],

Let's keep the original bug number too:

    "bug_numbers": [1150921, 1311926],

::: toolkit/components/telemetry/Histograms.json:4200
(Diff revision 2)
> -  "URLCLASSIFIER_COMPLETE_TIMEOUT": {
> +  "URLCLASSIFIER_COMPLETE_TIMEOUT2": {
>      "alert_emails": ["safebrowsing-telemetry@mozilla.org"],
> -    "expires_in_version": "56",
> +    "expires_in_version": "59",
>      "kind": "boolean",
> -    "bug_numbers": [1172688],
> -    "description": "This metric is recorded every time a gethash lookup is performed, `true` is recorded if the lookup times out."
> +    "keyed": true,
> +    "bug_numbers": [1311926],

Let's keep the original bug number too:

    "bug_numbers": [1172688, 1311926],
Attachment #8824337 - Flags: review?(francois) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/393ac0555624
Add telemetry to measure gethash error and gethash timeout rate for V2 and V4. r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/393ac0555624
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.