Closed
Bug 837202
Opened 11 years ago
Closed 10 years ago
Enable metrics on application reputation check
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: mmc, Assigned: mmc)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
8.00 KB,
patch
|
Paolo
:
review+
Yoric
:
review+
|
Details | Diff | Splinter Review |
It doesn't do much good to query for downloaded malware if we can't tell what benefit it has to users. At a minimum, we need to know: - How many downloads are taking place - How many of those are executable - How many of the returned verdicts are malware - How often queries fail That way, if someone asks what good this security feature is, our answer can be "we protected X downloads/daily and prevented Y attacks."
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mmc
Blocks: downloadprotection
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8339532 -
Flags: review?(paolo.mozmail)
Comment 2•10 years ago
|
||
Comment on attachment 8339532 [details] [diff] [review] Add telemetry for application reputation check ( Review of attachment 8339532 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Histograms.json @@ +26,5 @@ > + "APPLICATION_REPUTATION_RESULTS": { > + "kind": "enumerated", > + "n_values": 32, > + "description": "Results from performing application reputation lookups" > + }, I doubt this is the correct type of telemetry probe for what we're trying to achieve here, since it will be difficult to get numbers out of the possible combinations. Separate counters can be better analyzed. You may ask for feedback from Taras or someone who worked on Telemetry as well.
Attachment #8339532 -
Flags: review?(paolo.mozmail) → feedback?(taras.mozilla)
Comment 3•10 years ago
|
||
Comment on attachment 8339532 [details] [diff] [review] Add telemetry for application reputation check ( This doesn't look like what you want. Please talk to David about what you want to measure, he'll guide you.
Attachment #8339532 -
Flags: feedback?(taras.mozilla) → feedback?(dteller)
Assignee | ||
Comment 4•10 years ago
|
||
Thanks, Taras. David, what I want is counts for - The total number of downloads for which this check is called - Number of good and bad verdicts - Number of times a verdict was found locally, using local block and allow lists - Number of times a verdict was found remotely, successfully. This patch simply assigns a bit to each one of those conditions (there are 5), generating 2^5 = 32 state conditions. If the telemetry output is simply a histogram of counts for state values 0-31, then that's what I need to figure out all the metrics above. Thanks, Monica
Comment 5•10 years ago
|
||
If I understand correctly, you will probably be better off with one histogram per condition. Do you really need to know, say, how many times a single verdict was found simultaneously locally and remotely?
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] <currently on training, will be back on Friday 6th> from comment #5) > If I understand correctly, you will probably be better off with one > histogram per condition. Do you really need to know, say, how many times a > single verdict was found simultaneously locally and remotely? Well, the reason I put it all in one is that I don't want to think about it :) If I suddenly introduce a bug where all of the remote verdicts are coming back MALWARE then yeah, I want to know about it. I want to be able to calculate things like - % of downloads found locally on the allowlist - % of downloads found locally on the blocklist - % of downloads ineligible for check (because they're not executable, or whatever) - % of downloads found remotely with server errors - % of downloads found remotely with MALWARE (should never also have a server error, because we fail open) - % of downloads found remotely with OK verdict (could have server errors) without having to think through which combinations are valid (and more importantly, be able to detect when we get an invalid combination that indicates a bug). Thanks, Monica
Assignee | ||
Comment 7•10 years ago
|
||
So, should I rewrite with multiple histograms, or just keep this as is? Thanks, Monica
Flags: needinfo?(dteller)
Comment 8•10 years ago
|
||
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #6) > (and more importantly, be able to detect when we get an invalid combination that > indicates a bug). Telemetry is not the right tool for detecting bugs. If you think any code path is invalid, you can guard it using assertions. That said, I think you can easily answer all your questions using one histogram per condition. An enumeration will simply not give you the answers you are looking for, without some unusual processing on the data.
Assignee | ||
Comment 9•10 years ago
|
||
All right, I will re-write. Thanks, Monica
Flags: needinfo?(dteller)
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8339532 -
Attachment is obsolete: true
Attachment #8339532 -
Flags: feedback?(dteller)
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8344809 [details] [diff] [review] Add telemetry for application reputation check ( Review of attachment 8344809 [details] [diff] [review]: ----------------------------------------------------------------- Not sure if David needs to review all telemetry patches or not. Please take another look. Now there are 4 separate histograms and many more calls to Accumulate.
Attachment #8344809 -
Flags: review?(paolo.mozmail)
Attachment #8344809 -
Flags: feedback?(dteller)
Comment 12•10 years ago
|
||
Comment on attachment 8344809 [details] [diff] [review] Add telemetry for application reputation check ( Review of attachment 8344809 [details] [diff] [review]: ----------------------------------------------------------------- Just a couple of comments, David might want to do a more thorough review of the Telemetry probes. ::: toolkit/components/downloads/ApplicationReputation.cpp @@ +145,5 @@ > nsCString block_list; > Preferences::GetCString(PREF_DOWNLOAD_BLOCK_TABLE, &block_list); > if (FindInReadable(tables, block_list)) { > + mozilla::Telemetry::Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_LOCAL, > + BLOCK_LIST); Maybe there could be a third state here for the case where there is no match, to help in calculating the percentages? @@ +288,5 @@ > nsresult > PendingLookup::OnStopRequestInternal(nsIRequest *aRequest, > nsISupports *aContext, > nsresult aResult, > bool* aShouldBlock) { Unrelated to telemetry, I just noticed we forgot to check aResult here: it will be a failure code if the request failed. I think the status code check below catches this in any case, so there is no real issue here, but we should probably check aResult first for correctness. ::: toolkit/components/telemetry/Histograms.json @@ +25,5 @@ > }, > + "APPLICATION_REPUTATION_VERDICT": { > + "kind": "flag", > + "description": "Application reputation verdict (0 is OK)" > + }, I think this should be "boolean": https://developer.mozilla.org/en-US/docs/Performance/Adding_a_new_Telemetry_probe#Choosing_a_Histogram_Type
Attachment #8344809 -
Flags: review?(paolo.mozmail) → feedback+
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8344809 -
Attachment is obsolete: true
Attachment #8344809 -
Flags: feedback?(dteller)
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8344876 [details] [diff] [review] Add telemetry for application reputation check ( Review of attachment 8344876 [details] [diff] [review]: ----------------------------------------------------------------- Address Paolo's comments. David, would you like to review? Thanks, Monica
Attachment #8344876 -
Flags: review?(paolo.mozmail)
Attachment #8344876 -
Flags: feedback?
Comment 15•10 years ago
|
||
Comment on attachment 8344876 [details] [diff] [review] Add telemetry for application reputation check ( Looks good to me, let's wait for David's review on the probes.
Attachment #8344876 -
Flags: review?(paolo.mozmail)
Attachment #8344876 -
Flags: review?(dteller)
Attachment #8344876 -
Flags: review+
Attachment #8344876 -
Flags: feedback?
Comment on attachment 8344876 [details] [diff] [review] Add telemetry for application reputation check ( Review of attachment 8344876 [details] [diff] [review]: ----------------------------------------------------------------- Looks good and much simpler, thanks. Sorry about the delay, I was on a combo of formation/MozEdu/PTO/sick. ::: toolkit/components/downloads/ApplicationReputation.cpp @@ +77,5 @@ > + */ > + static const uint32_t SERVER_RESPONSE_VALID = 0; > + static const uint32_t SERVER_RESPONSE_FAILED = 1; > + static const uint32_t SERVER_RESPONSE_INVALID = 2; > + Out of curiosity, why are these not enums? @@ +123,5 @@ > > nsresult > PendingLookup::OnComplete(bool shouldBlock, nsresult rv) { > + mozilla::Telemetry::Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_SHOULD_BLOCK, > + shouldBlock); Looks like an indentation issue, here and in the other instances of mozilla::Telemetry::Accumulate. Also, please add a using |using mozilla::Telemetry;| close to the |using mozilla::Preferences;|, this will make code much more readable. ::: toolkit/components/telemetry/Histograms.json @@ +29,5 @@ > + }, > + "APPLICATION_REPUTATION_LOCAL": { > + "kind": "enumerated", > + "n_values": 3, > + "description": "Application reputation local results (0 = ALLOW, 1=BLOCK, 2=NONE)" Nit: Please be consistent with spaces around "=".
Attachment #8344876 -
Flags: review?(dteller) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Thanks, David. > Out of curiosity, why are these not enums? They started out as bitmask positions :) Switched to enums. > @@ +123,5 @@ > > > > nsresult > > PendingLookup::OnComplete(bool shouldBlock, nsresult rv) { > > + mozilla::Telemetry::Accumulate(mozilla::Telemetry::APPLICATION_REPUTATION_SHOULD_BLOCK, > > + shouldBlock); > > Looks like an indentation issue, here and in the other instances of > mozilla::Telemetry::Accumulate. I've gotten conflicting feedback about this (some reviewers like dangling params to line up on parens or commas) but will implement a last reviewer trumps in this case :) > Also, please add a using |using mozilla::Telemetry;| close to the |using > mozilla::Preferences;|, this will make code much more readable. Added using mozilla::Telemetry::Accumulate since importing an entire namespace generates a warning. > ::: toolkit/components/telemetry/Histograms.json > @@ +29,5 @@ > > + }, > > + "APPLICATION_REPUTATION_LOCAL": { > > + "kind": "enumerated", > > + "n_values": 3, > > + "description": "Application reputation local results (0 = ALLOW, 1=BLOCK, 2=NONE)" > > Nit: Please be consistent with spaces around "=". Fixed and checked in: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b911ca4dd8
Comment 18•10 years ago
|
||
Backed out for OSX Werror bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4441403e58 https://tbpl.mozilla.org/php/getParsedLog.php?id=31885481&tree=Mozilla-Inbound
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18) > Backed out for OSX Werror bustage. > https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4441403e58 > > https://tbpl.mozilla.org/php/getParsedLog.php?id=31885481&tree=Mozilla- > Inbound Ugh, I'm sorry. I'm going to install clang on my Linux box so I don't cause this problem again. Here's a try run in the meantime: https://tbpl.mozilla.org/?tree=Try&rev=887fae46e65e
Assignee | ||
Comment 20•10 years ago
|
||
Try was green on Mac, so one more time: https://hg.mozilla.org/integration/mozilla-inbound/rev/7aaf8db2d681
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7aaf8db2d681
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•