Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2381233002:
Metrics - Add histograms/README.md (Closed)
DescriptionMetrics - Add histograms/README.md
Add a README file in the metrics histograms directory indication best
practices for emitting and documenting histograms.
I borrowed a sentence or two here or there from header files where I thought the header files explained things well. I hope the original authors of those words don't mind this repurposing.
I decided not to talk about user actions here.
There is nothing here about how to analyze the histogram results. The
current scope for this document felt right without that. We may want
to consider adding that later, probably to another doc.
I did not yet check how this rendered in markdown. I thought I'd
send it for review now, as I likely won't be able to double-check the
markdown for some time (another day?).
BUG=
Committed: https://crrev.com/2b5f7e00b10ebf583312626d2f269ab2edb839cf
Cr-Commit-Position: refs/heads/master@{#422536}
Patch Set 1 #
Total comments: 14
Patch Set 2 : added a "directly measure" section #Patch Set 3 : Rob's suggestions #Patch Set 4 : rebase #Patch Set 5 : restore comment removed during rebasing #Patch Set 6 : fix typo #Patch Set 7 : fix another typo #
Messages
Total messages: 28 (13 generated)
Description was changed from ========== Metrics - Add histograms/README.md Add a README file in the metrics histograms directory indication best practices for emitting and documenting histograms. I decided not to talk about user actions here. There is nothing here about how to analyze the histogram results. The current scope for this document felt right without that. We may want to consider adding that later, probably to another doc. I did not yet check how this rendered in markdown. I thought I'd send it for review now, as I likely won't be able to double-check the markdown for some time (another day?). BUG= ========== to ========== Metrics - Add histograms/README.md Add a README file in the metrics histograms directory indication best practices for emitting and documenting histograms. I decided not to talk about user actions here. There is nothing here about how to analyze the histogram results. The current scope for this document felt right without that. We may want to consider adding that later, probably to another doc. I did not yet check how this rendered in markdown. I thought I'd send it for review now, as I likely won't be able to double-check the markdown for some time (another day?). BUG= ==========
mpearson@chromium.org changed reviewers: + rkaplow@chromium.org
Description was changed from ========== Metrics - Add histograms/README.md Add a README file in the metrics histograms directory indication best practices for emitting and documenting histograms. I decided not to talk about user actions here. There is nothing here about how to analyze the histogram results. The current scope for this document felt right without that. We may want to consider adding that later, probably to another doc. I did not yet check how this rendered in markdown. I thought I'd send it for review now, as I likely won't be able to double-check the markdown for some time (another day?). BUG= ========== to ========== Metrics - Add histograms/README.md Add a README file in the metrics histograms directory indication best practices for emitting and documenting histograms. I borrowed a sentence or two here or there from header files where I thought the header files explained things well. I hope the original authors of those words don't mind this repurposing. I decided not to talk about user actions here. There is nothing here about how to analyze the histogram results. The current scope for this document felt right without that. We may want to consider adding that later, probably to another doc. I did not yet check how this rendered in markdown. I thought I'd send it for review now, as I likely won't be able to double-check the markdown for some time (another day?). BUG= ==========
Rob, Please take a look. thanks, mark
FYI, I added another paragraph since I sent the notice for the review. --mark
Looks good - just a few suggestions for additional information mostly. There's a couple other topics on my mind that should be added here, but I'm happy to add them after this review and send to you. https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... File tools/metrics/histograms/README.md (right): https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:1: # Histogram Guidelines You could put the pointer to this file from histogram_macros.h, and optionally at the top of histograms.xml https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:25: visited tile, click a bookmark, etc.) would make a good enumerated histogram. you may also want to add something like: "It is often the case that an enum histogram maps to one of N outcomes from a particular action. If the total count of your histogram (i.e. the sum across all buckets) is something meaningful, that is generally a good sign. In our example, the total count is the number of actions on the New Tab Page". or maybe a slightly less wordy version of this. https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:33: Also, please explicitly set enum values `= 0`, `= 1`, `= 2`, etc. This makes Probably worth mentioning that we allow appending to the enum by not reordering, etc. https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:37: ### Count Histograms: Choosing Min and Max should we have a small introduction to counts here, and mention we have specialized macros for common count types (memory, timing, etc.?) https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:42: percentile. Err on the side of too large a range versus too short a range. Also option, you can put something like: "One rule of thumb is at most 1% of samples should be in the overflow bucket. This allows analysis of the 99th percentile". https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:109: necessary. Maybe add: It is good practice to note caveats associated with your histogram in this section, such as which platforms are supported. https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:119: as obsolete. It would be bad if someone to accidentally reused your old obsolete, with the associated date or milestone in the obsolete tag entry. Maybe also: "If your histogram is being replaced by a new version, we suggest noting that in the previous histogram's description, with a pointer to the new one."
Please take another look. thanks, mark https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... File tools/metrics/histograms/README.md (right): https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:1: # Histogram Guidelines On 2016/09/30 17:12:02, rkaplow wrote: > You could put the pointer to this file from histogram_macros.h, and optionally > at the top of histograms.xml Done for both. https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:25: visited tile, click a bookmark, etc.) would make a good enumerated histogram. On 2016/09/30 17:12:01, rkaplow wrote: > you may also want to add something like: > > "It is often the case that an enum histogram maps to one of N outcomes from a > particular action. If the total count of your histogram (i.e. the sum across all > buckets) is something meaningful, that is generally a good sign. In our example, > the total count is the number of actions on the New Tab Page". > > or maybe a slightly less wordy version of this. Added a less wordy version. :-) Thanks for the first draft. :-) https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:33: Also, please explicitly set enum values `= 0`, `= 1`, `= 2`, etc. This makes On 2016/09/30 17:12:02, rkaplow wrote: > Probably worth mentioning that we allow appending to the enum by not reordering, > etc. Done. https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:37: ### Count Histograms: Choosing Min and Max On 2016/09/30 17:12:01, rkaplow wrote: > should we have a small introduction to counts here, and mention we have > specialized macros for common count types (memory, timing, etc.?) Done. Seems a little wordy though. :-| *shrug* https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:42: percentile. Err on the side of too large a range versus too short a range. On 2016/09/30 17:12:02, rkaplow wrote: > Also option, you can put something like: > > "One rule of thumb is at most 1% of samples should be in the overflow bucket. > This allows analysis of the 99th percentile". > Added, and removed an existing clause. Then added another sentence too. https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:109: necessary. On 2016/09/30 17:12:01, rkaplow wrote: > Maybe add: > > It is good practice to note caveats associated with your histogram in this > section, such as which platforms are supported. I don't think the list of platforms should be added in general. Added that sentence with a caveat. https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... tools/metrics/histograms/README.md:119: as obsolete. It would be bad if someone to accidentally reused your old On 2016/09/30 17:12:02, rkaplow wrote: > obsolete, with the associated date or milestone in the obsolete tag entry. > > Maybe also: > "If your histogram is being replaced by a new version, we suggest noting that in > the previous histogram's description, with a pointer to the new one." Both good suggestions. Integrated them both.
On 2016/09/30 22:40:36, Mark P wrote: > Please take another look. > > thanks, > mark > > https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... > File tools/metrics/histograms/README.md (right): > > https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... > tools/metrics/histograms/README.md:1: # Histogram Guidelines > On 2016/09/30 17:12:02, rkaplow wrote: > > You could put the pointer to this file from histogram_macros.h, and optionally > > at the top of histograms.xml > > Done for both. > > https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... > tools/metrics/histograms/README.md:25: visited tile, click a bookmark, etc.) > would make a good enumerated histogram. > On 2016/09/30 17:12:01, rkaplow wrote: > > you may also want to add something like: > > > > "It is often the case that an enum histogram maps to one of N outcomes from a > > particular action. If the total count of your histogram (i.e. the sum across > all > > buckets) is something meaningful, that is generally a good sign. In our > example, > > the total count is the number of actions on the New Tab Page". > > > > or maybe a slightly less wordy version of this. > > Added a less wordy version. :-) > > Thanks for the first draft. :-) > > https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... > tools/metrics/histograms/README.md:33: Also, please explicitly set enum values > `= 0`, `= 1`, `= 2`, etc. This makes > On 2016/09/30 17:12:02, rkaplow wrote: > > Probably worth mentioning that we allow appending to the enum by not > reordering, > > etc. > > Done. > > https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... > tools/metrics/histograms/README.md:37: ### Count Histograms: Choosing Min and > Max > On 2016/09/30 17:12:01, rkaplow wrote: > > should we have a small introduction to counts here, and mention we have > > specialized macros for common count types (memory, timing, etc.?) > > Done. Seems a little wordy though. :-| *shrug* > > https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... > tools/metrics/histograms/README.md:42: percentile. Err on the side of too large > a range versus too short a range. > On 2016/09/30 17:12:02, rkaplow wrote: > > Also option, you can put something like: > > > > "One rule of thumb is at most 1% of samples should be in the overflow bucket. > > This allows analysis of the 99th percentile". > > > > Added, and removed an existing clause. Then added another sentence too. > > https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... > tools/metrics/histograms/README.md:109: necessary. > On 2016/09/30 17:12:01, rkaplow wrote: > > Maybe add: > > > > It is good practice to note caveats associated with your histogram in this > > section, such as which platforms are supported. > > I don't think the list of platforms should be added in general. Added that > sentence with a caveat. Sure - I'm mostly thinking of the case of histograms only working for a single platform (relatively common, especially in iOS) , which should be noted. > > https://codereview.chromium.org/2381233002/diff/1/tools/metrics/histograms/RE... > tools/metrics/histograms/README.md:119: as obsolete. It would be bad if someone > to accidentally reused your old > On 2016/09/30 17:12:02, rkaplow wrote: > > obsolete, with the associated date or milestone in the obsolete tag entry. > > > > Maybe also: > > "If your histogram is being replaced by a new version, we suggest noting that > in > > the previous histogram's description, with a pointer to the new one." > > Both good suggestions. Integrated them both.
lgtm great! thanks
The CQ bit was checked by mpearson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mpearson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2381233002/#ps80001 (title: "restore comment removed during rebasing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mpearson@chromium.org changed reviewers: + asvitkine@chromium.org
+alexei as base/metrics OWNER Rob has already lgtmed this. --mark
lgtm
The CQ bit was checked by mpearson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2381233002/#ps120001 (title: "fix another typo")
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 ========== Metrics - Add histograms/README.md Add a README file in the metrics histograms directory indication best practices for emitting and documenting histograms. I borrowed a sentence or two here or there from header files where I thought the header files explained things well. I hope the original authors of those words don't mind this repurposing. I decided not to talk about user actions here. There is nothing here about how to analyze the histogram results. The current scope for this document felt right without that. We may want to consider adding that later, probably to another doc. I did not yet check how this rendered in markdown. I thought I'd send it for review now, as I likely won't be able to double-check the markdown for some time (another day?). BUG= ========== to ========== Metrics - Add histograms/README.md Add a README file in the metrics histograms directory indication best practices for emitting and documenting histograms. I borrowed a sentence or two here or there from header files where I thought the header files explained things well. I hope the original authors of those words don't mind this repurposing. I decided not to talk about user actions here. There is nothing here about how to analyze the histogram results. The current scope for this document felt right without that. We may want to consider adding that later, probably to another doc. I did not yet check how this rendered in markdown. I thought I'd send it for review now, as I likely won't be able to double-check the markdown for some time (another day?). BUG= ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Metrics - Add histograms/README.md Add a README file in the metrics histograms directory indication best practices for emitting and documenting histograms. I borrowed a sentence or two here or there from header files where I thought the header files explained things well. I hope the original authors of those words don't mind this repurposing. I decided not to talk about user actions here. There is nothing here about how to analyze the histogram results. The current scope for this document felt right without that. We may want to consider adding that later, probably to another doc. I did not yet check how this rendered in markdown. I thought I'd send it for review now, as I likely won't be able to double-check the markdown for some time (another day?). BUG= ========== to ========== Metrics - Add histograms/README.md Add a README file in the metrics histograms directory indication best practices for emitting and documenting histograms. I borrowed a sentence or two here or there from header files where I thought the header files explained things well. I hope the original authors of those words don't mind this repurposing. I decided not to talk about user actions here. There is nothing here about how to analyze the histogram results. The current scope for this document felt right without that. We may want to consider adding that later, probably to another doc. I did not yet check how this rendered in markdown. I thought I'd send it for review now, as I likely won't be able to double-check the markdown for some time (another day?). BUG= Committed: https://crrev.com/2b5f7e00b10ebf583312626d2f269ab2edb839cf Cr-Commit-Position: refs/heads/master@{#422536} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2b5f7e00b10ebf583312626d2f269ab2edb839cf Cr-Commit-Position: refs/heads/master@{#422536} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
