Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574533003:
metrics: improve histogram docs (Closed)
Descriptionmetrics: improve histogram docs
BUG=673501
Committed: https://crrev.com/52b9aba37043a56f2ca0f01b0a07a0cda629cc40
Cr-Commit-Position: refs/heads/master@{#438446}
Patch Set 1 #Patch Set 2 : metrics: improve histogram docs #
Total comments: 7
Patch Set 3 : address feedback #Messages
Total messages: 13 (5 generated)
vapier@chromium.org changed reviewers: + asvitkine@chromium.org, mpearson@chromium.org
you can see example rendering here: https://chromium.googlesource.com/chromium/src/+/3ee3a5cbfb274cc9e3376371e136...
Thanks for linkifying those histograms.xml references. https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/README.md (right): https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/README.md:5: of histograms: This table of contents above the actual table of contents makes me nervous. I think a lot of people will come here, say, I want an enum histogram, and click there and just read that section, not the general guidelines for all histograms. Perhaps we merge this list into a sentence with no links, e.g. after "histograms: enumerated histograms, count histograms (for arbitrary numbers), and sparse histograms (for anything when ...)." https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/README.md:10: precision is important over a wide range is large and/or the range is not omit: "is large" https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/README.md:141: ## Adding New Histograms I'm not sure this section adds anything. The first sentence is misleading (you can emit histograms without adding them to histograms.xml). And the second sentence basically says "keep reading the next section." If you still want to add a section like this, consider (1) putting it above the "Revising Histograms" header (2) make it clear: A call it to a histogram macro is all that is needed to emit to histogram; no other declarations or statements are required. However, if this histogram is to be reported to UMA (i.e., is not a local histogram), be sure to document it. (make the last part a link to the documenting section). But I am okay with omitting this section entirely too.
https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/README.md (right): https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/README.md:5: of histograms: ok https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/README.md:10: precision is important over a wide range is large and/or the range is not done https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/README.md:141: ## Adding New Histograms what got me started here is that i needed to add a new histogram but there was no high level info on it. it seems like even if you read this entire document, there's no clear guidelines for what exactly you need to do. i can split out this improvement to a new CL though so we can land the rest and then debate the overall big picture.
updated example rendering: https://chromium.googlesource.com/chromium/src/+/4b0b60ff7e6e77ee610778401263...
lgtm https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/README.md (right): https://codereview.chromium.org/2574533003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/README.md:141: ## Adding New Histograms On 2016/12/13 22:15:29, vapier wrote: > what got me started here is that i needed to add a new histogram but there was > no high level info on it. it seems like even if you read this entire document, > there's no clear guidelines for what exactly you need to do. > > i can split out this improvement to a new CL though so we can land the rest and > then debate the overall big picture. Okay.
The CQ bit was checked by vapier@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481692058981140,
"parent_rev": "36551b6f79d7ab7ac05c27e17ac8a496784b179c", "commit_rev":
"a3cc103444b61a92f8a5fcd70924331e5e39e9cd"}
Message was sent while issue was closed.
Description was changed from ========== metrics: improve histogram docs BUG=673501 ========== to ========== metrics: improve histogram docs BUG=673501 Review-Url: https://codereview.chromium.org/2574533003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== metrics: improve histogram docs BUG=673501 Review-Url: https://codereview.chromium.org/2574533003 ========== to ========== metrics: improve histogram docs BUG=673501 Committed: https://crrev.com/52b9aba37043a56f2ca0f01b0a07a0cda629cc40 Cr-Commit-Position: refs/heads/master@{#438446} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/52b9aba37043a56f2ca0f01b0a07a0cda629cc40 Cr-Commit-Position: refs/heads/master@{#438446} |
