Closed
Bug 1311926
Opened 8 years ago
Closed 7 years ago
Add telemetry to measure gethash error and gethash timeout rate for V2 and V4
Categories
(Toolkit :: Safe Browsing, defect, P2)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: dlee, 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.
Reporter | ||
Updated•8 years ago
|
Priority: -- → P2
Updated•8 years ago
|
Blocks: SafeBrowsingv4Telemetry
Reporter | ||
Comment 1•8 years ago
|
||
As francois suggested in Bug 1311910 Comment 5,, we should add following as keyed histogram: `URLCLASSIFIER_COMPLETE_TIMEOUT` `URLCLASSIFIER_COMPLETE_REMOTE_STATUS`
Reporter | ||
Updated•7 years ago
|
Whiteboard: #sbv4-m4 → #sbv4-m5
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tnguyen
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-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/#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-
Assignee | ||
Comment 4•7 years ago
|
||
Thanks Francois for your review.
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca475671e654bcdc9ce247ab09be432ae67b693d
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/393ac0555624
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•