Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2409793002:
Add detailed UMA metrics for v4_store, broken down by storeid. (Closed)
|
Created:
4 years, 2 months ago by vakh (use Gerrit instead) Modified:
4 years, 2 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd detailed UMA metrics for v4_store, broken down by storeid.
BUG=651911, 543161
TBR=nparker
Committed: https://crrev.com/36a98c5d86965c361330863904c19435e278a4c0
Cr-Commit-Position: refs/heads/master@{#424946}
Patch Set 1 #Patch Set 2 : git pull #
Total comments: 11
Patch Set 3 : nparker@ review #Patch Set 4 : Mark old histograms as obsolete instead of deleting them #
Total comments: 4
Messages
Total messages: 41 (22 generated)
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vakh@chromium.org changed reviewers: + holte@chromium.org, nparker@chromium.org
vakh@chromium.org changed reviewers: + shess@chromium.org
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
git pull
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2409793002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2409793002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51211: -<histogram name="SafeBrowsing.V4DecodeAdditionsResult" Don't remove the old histogram descriptions, since they will still be reported by older versions of chrome. Instead, mark them as obsolete.
https://codereview.chromium.org/2409793002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2409793002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51211: -<histogram name="SafeBrowsing.V4DecodeAdditionsResult" On 2016/10/10 22:04:28, Steven Holte wrote: > Don't remove the old histogram descriptions, since they will still be reported > by older versions of chrome. Instead, mark them as obsolete. The old ones were added fairly recently by me and I don't intend to track them going forward. Even in this case should they be marked as obsolete?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Have you tested this by checking chrome://histograms to ensure it all looks as you expect? It's also possible to write tests for UMA metrics. Up to you. https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:26: const char kDecodeAdditions[] = ".DecodeAdditions"; I think it'd be clearer to group these into suffixes and prefixes (or "base metrics"). Actually, I think this is complex enough that'd it'd be good to spell out all in comments all the metrics this file reports, in a compressed form. e.g. SafeBrowsing.V4ProcessFullUpdate[.{soc_eng,malware,..}].{Result,Time,Decode..} ... Or SafeBrowsing.${PROCESS}.${OPT_LIST}.${SUB_PROCESS} and spell out when each of those could contain https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:92: UMA_HISTOGRAM_LONG_TIMES("SafeBrowsing.V4AddUnlumpedHashes.Time", time); Should this one also vary by store name? https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:98: RecordEnumWithAndWithoutSuffix(base_metric + ".ApplyUpdate", result, use a const string here too? https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:133: RecordTimeWithAndWithoutSuffix(base_metric + ".MergeUpdate", time, file_path); And here too? Or, are you just using constants for strings that are used > once?
https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:26: const char kDecodeAdditions[] = ".DecodeAdditions"; On 2016/10/11 20:51:48, Nathan Parker wrote: > I think it'd be clearer to group these into suffixes and prefixes (or "base > metrics"). > > Actually, I think this is complex enough that'd it'd be good to spell out all in > comments all the metrics this file reports, in a compressed form. e.g. > > SafeBrowsing.V4ProcessFullUpdate[.{soc_eng,malware,..}].{Result,Time,Decode..} > ... > > Or > SafeBrowsing.${PROCESS}.${OPT_LIST}.${SUB_PROCESS} > and spell out when each of those could contain Done. https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:92: UMA_HISTOGRAM_LONG_TIMES("SafeBrowsing.V4AddUnlumpedHashes.Time", time); On 2016/10/11 20:51:48, Nathan Parker wrote: > Should this one also vary by store name? No, I intend to delete this histogram actually. I'm adding it to justify its deletion. It takes 0-1 ms per operation so probably not worth recording at all. https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:98: RecordEnumWithAndWithoutSuffix(base_metric + ".ApplyUpdate", result, On 2016/10/11 20:51:48, Nathan Parker wrote: > use a const string here too? Only used in this one place so defined inline. https://codereview.chromium.org/2409793002/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:133: RecordTimeWithAndWithoutSuffix(base_metric + ".MergeUpdate", time, file_path); On 2016/10/11 20:51:48, Nathan Parker wrote: > And here too? Or, are you just using constants for strings that are used > once? Yup, except kProcessFullUpdate but defining it at the top to keep it consistent with kProcessPartialUpdate.
nparker@ review
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/11 20:51:48, Nathan Parker wrote: > Have you tested this by checking chrome://histograms to ensure it all looks as > you expect? Yes I have. See: https://paste.googleplex.com/4608922040139776 > > It's also possible to write tests for UMA metrics. Up to you. Just doing manual tests right now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2409793002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2409793002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51211: -<histogram name="SafeBrowsing.V4DecodeAdditionsResult" On 2016/10/10 22:15:59, vakh (Varun Khaneja) wrote: > On 2016/10/10 22:04:28, Steven Holte wrote: > > Don't remove the old histogram descriptions, since they will still be reported > > by older versions of chrome. Instead, mark them as obsolete. > > The old ones were added fairly recently by me and I don't intend to track them > going forward. Even in this case should they be marked as obsolete? Leaving them in at least means that these don't show up as unknown values in histogram eraser. This file is overdue for some cleanup, so we'll probably need to do something to trim obsolete histograms soon, but if you leave them in as obsolete they can get picked up in whatever that cleanup ends up doing.
Mark old histograms as obsolete instead of deleting them
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/12 01:03:48, Steven Holte wrote: > https://codereview.chromium.org/2409793002/diff/20001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (left): > > https://codereview.chromium.org/2409793002/diff/20001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:51211: -<histogram > name="SafeBrowsing.V4DecodeAdditionsResult" > On 2016/10/10 22:15:59, vakh (Varun Khaneja) wrote: > > On 2016/10/10 22:04:28, Steven Holte wrote: > > > Don't remove the old histogram descriptions, since they will still be > reported > > > by older versions of chrome. Instead, mark them as obsolete. > > > > The old ones were added fairly recently by me and I don't intend to track them > > going forward. Even in this case should they be marked as obsolete? > > Leaving them in at least means that these don't show up as unknown values in > histogram eraser. This file is overdue for some cleanup, so we'll probably need > to do something to trim obsolete histograms soon, but if you leave them in as > obsolete they can get picked up in whatever that cleanup ends up doing. Fair enough. Done. PTAL.
histograms lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add detailed UMA metrics for v4_store, broken down by storeid. BUG=651911, 543161 ========== to ========== Add detailed UMA metrics for v4_store, broken down by storeid. BUG=651911, 543161 TBR=nparker ==========
Thanks holte@ Submitting this with TBR=nparker since I've addressed his comments and 3 of my pending CLs are going to have merge conflicts with this so I want to submit this asap.
The CQ bit was checked by vakh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add detailed UMA metrics for v4_store, broken down by storeid. BUG=651911, 543161 TBR=nparker ========== to ========== Add detailed UMA metrics for v4_store, broken down by storeid. BUG=651911, 543161 TBR=nparker ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add detailed UMA metrics for v4_store, broken down by storeid. BUG=651911, 543161 TBR=nparker ========== to ========== Add detailed UMA metrics for v4_store, broken down by storeid. BUG=651911, 543161 TBR=nparker Committed: https://crrev.com/36a98c5d86965c361330863904c19435e278a4c0 Cr-Commit-Position: refs/heads/master@{#424946} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/36a98c5d86965c361330863904c19435e278a4c0 Cr-Commit-Position: refs/heads/master@{#424946}
Message was sent while issue was closed.
lgtm Sorry for the delay. I'll LGTM, but I think the histograms.xml needs a fix. https://codereview.chromium.org/2409793002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2409793002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:40: // in order, from parts 1, 2, and 3. nit: You're missing part 2.5, which is the store name. https://codereview.chromium.org/2409793002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2409793002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51510: +<histogram name="SafeBrowsing.V4StoreRead.Result" I think all the .Result and .Time metrics need to be expanded by the list of store names, ya? We should add a comment to the list of store names in the .cc to remember to update the histograms. You should be able to accomplish this with <histogram_suffixes .. separator="." ordering="prefix"> (I think...)
Message was sent while issue was closed.
See: https://codereview.chromium.org/2423523003 https://codereview.chromium.org/2409793002/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_store.cc (right): https://codereview.chromium.org/2409793002/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_store.cc:40: // in order, from parts 1, 2, and 3. On 2016/10/14 20:48:32, Nathan Parker wrote: > nit: You're missing part 2.5, which is the store name. Done. https://codereview.chromium.org/2409793002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2409793002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51510: +<histogram name="SafeBrowsing.V4StoreRead.Result" On 2016/10/14 20:48:33, Nathan Parker wrote: > I think all the .Result and .Time metrics need to be expanded by the list of > store names, ya? We should add a comment to the list of store names in the .cc > to remember to update the histograms. > > You should be able to accomplish this with <histogram_suffixes .. separator="." > ordering="prefix"> (I think...) When using ordering="prefix", it adds the specified suffix after the first period character in the histogram name so that wouldn't have worked for us. See: https://cs.chromium.org/chromium/src/tools/metrics/actions/action_utils.py?q=... I've added the histograms manually. See: https://codereview.chromium.org/2423523003 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
