Closed Bug 1297865 Opened 4 years ago Closed 4 years ago

Update Safe Browsing histograms

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: francois, Assigned: francois)

References

Details

Attachments

(4 files)

Many of the Safe Browsing histograms don't include alert email addresses.

Also, the description of some of them is lacking (e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1291472#c23).
I submitted a Service Now ticket (RITM0046933) to create a safebrowsing-telemetry@mozilla.org list. I'll use that in the alert field so that we don't need to update histograms.json whenever someone new wants to get added.
I could not find any reports for URLCLASSIFIER_PS_FAILURE in all of the releases (Nightlies, Releases and Betas) I looked.

It's only used in this goto: http://searchfox.org/mozilla-central/rev/44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/toolkit/components/url-classifier/LookupCache.cpp#492

I suggest we take it out.
Comment on attachment 8785089 [details]
Bug 1297865 - Remove unused URLCLASSIFIER_PS_FAILURE probe.

https://reviewboard.mozilla.org/r/74408/#review73236
Attachment #8785089 - Flags: review?(gpascutto) → review+
Comment on attachment 8785090 [details]
Bug 1297865 - Extend Safe Browsing telemetry probes we are still using.

https://reviewboard.mozilla.org/r/74410/#review73244

::: toolkit/components/telemetry/Histograms.json:3674
(Diff revision 1)
>      "bug_numbers": [1150921],
>      "description": "Server HTTP status code from remote SafeBrowsing gethash lookups. (0=1xx, 1=200, 2=2xx, 3=204, 4=3xx, 5=400, 6=4xx, 7=403, 8=404, 9=408, 10=413, 11=5xx, 12=502|504|511, 13=503, 14=505, 15=Other)"
>    },
>    "URLCLASSIFIER_COMPLETE_TIMEOUT": {
>      "alert_emails": ["gcp@mozilla.com", "francois@mozilla.com"],
> -    "expires_in_version": "52",
> +    "expires_in_version": "56",

Maybe make this never? It's opt-in for release anyway, and we'll want to know when completes fail. I guess if those are no longer really a thing for SBv4 we won't need it after that switch, though.
Attachment #8785090 - Flags: review+
Comment on attachment 8785091 [details]
Bug 1297865 - Add an email address to all Safe Browsing telemetry probes.

https://reviewboard.mozilla.org/r/74412/#review73256
Attachment #8785091 - Flags: review+
Comment on attachment 8785092 [details]
Bug 1297865 - Improve the description of Application Reputation telemetry probes.

https://reviewboard.mozilla.org/r/74414/#review73258
Attachment #8785092 - Flags: review?(gpascutto) → review+
Comment on attachment 8785089 [details]
Bug 1297865 - Remove unused URLCLASSIFIER_PS_FAILURE probe.

https://reviewboard.mozilla.org/r/74408/#review73718
Attachment #8785089 - Flags: review?(benjamin) → review+
Comment on attachment 8785090 [details]
Bug 1297865 - Extend Safe Browsing telemetry probes we are still using.

https://reviewboard.mozilla.org/r/74410/#review73722

::: toolkit/components/telemetry/Histograms.json:80
(Diff revision 1)
>      "kind": "enumerated",
>      "n_values": 3,
>      "description": "Application reputation remote status (0=OK, 1=FAIL, 2=INVALID)"
>    },
>    "APPLICATION_REPUTATION_SERVER_VERDICT": {
> -    "expires_in_version": "52",
> +    "expires_in_version": "56",

I presume that you've already proved that these are valuable metrics. Do you have a link to the report that you're using to monitor these?

::: toolkit/components/telemetry/Histograms.json:3629
(Diff revision 1)
>      "high": 1000,
>      "n_buckets": 10,
>      "description": "Time spent loading PrefixSet from file (ms)"
>    },
>    "URLCLASSIFIER_PS_FALLOCATE_TIME": {
> -    "expires_in_version": "default",
> +    "expires_in_version": "never",

What is your monitoring plan for this? Do you have a dashboard or some kind of alerting to notify you if this gets worse?
Flags: needinfo?(francois)
Comment on attachment 8785091 [details]
Bug 1297865 - Add an email address to all Safe Browsing telemetry probes.

https://reviewboard.mozilla.org/r/74412/#review73726
Attachment #8785091 - Flags: review?(benjamin) → review+
Comment on attachment 8785092 [details]
Bug 1297865 - Improve the description of Application Reputation telemetry probes.

https://reviewboard.mozilla.org/r/74414/#review73728
Attachment #8785092 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg [:bsmedberg] from comment #12)
> ::: toolkit/components/telemetry/Histograms.json:80
> (Diff revision 1)
> >      "kind": "enumerated",
> >      "n_values": 3,
> >      "description": "Application reputation remote status (0=OK, 1=FAIL, 2=INVALID)"
> >    },
> >    "APPLICATION_REPUTATION_SERVER_VERDICT": {
> > -    "expires_in_version": "52",
> > +    "expires_in_version": "56",
> 
> I presume that you've already proved that these are valuable metrics. Do you
> have a link to the report that you're using to monitor these?

I'm looking at these manually and discussing them with Google whenever they change.

> ::: toolkit/components/telemetry/Histograms.json:3629
> (Diff revision 1)
> >      "high": 1000,
> >      "n_buckets": 10,
> >      "description": "Time spent loading PrefixSet from file (ms)"
> >    },
> >    "URLCLASSIFIER_PS_FALLOCATE_TIME": {
> > -    "expires_in_version": "default",
> > +    "expires_in_version": "never",
> 
> What is your monitoring plan for this? Do you have a dashboard or some kind
> of alerting to notify you if this gets worse?

One of the other patches adds an alert email so that we can get notified if this gets worse.

I'm planning to work on better monitoring for Safe Browsing telemetry next quarter. This patch is just to clean up and preserve what we have at the moment.
Flags: needinfo?(francois)
Benjamin, do you need more information for the remaining review?
Flags: needinfo?(benjamin)
Comment on attachment 8785090 [details]
Bug 1297865 - Extend Safe Browsing telemetry probes we are still using.

https://reviewboard.mozilla.org/r/74410/#review77060

ok. Be aware that current alerting only really works properly for scalar values (performance data). But it seems that you're using it effectively. I'd love to see a good public dashboard out of the work you're planning for next quarter.
Attachment #8785090 - Flags: review?(benjamin) → review+
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/073c18f01cc1
Remove unused URLCLASSIFIER_PS_FAILURE probe. r=bsmedberg,gcp
https://hg.mozilla.org/integration/autoland/rev/b73338e635ca
Extend Safe Browsing telemetry probes we are still using. r=bsmedberg,gcp
https://hg.mozilla.org/integration/autoland/rev/144ae7fb5144
Add an email address to all Safe Browsing telemetry probes. r=bsmedberg,gcp
https://hg.mozilla.org/integration/autoland/rev/be483544504d
Improve the description of Application Reputation telemetry probes. r=bsmedberg,gcp
Flags: needinfo?(benjamin)
You need to log in before you can comment on or make changes to this bug.