Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815693004:
Add more UMA in CopylessPasteExtractor (Closed)
DescriptionAdd more UMA in CopylessPasteExtractor
- Enum of extraction status
- Histogram of extraction time
(those w/ JSON parsing results are counted separately)
BUG=693633
Review-Url: https://codereview.chromium.org/2815693004
Cr-Commit-Position: refs/heads/master@{#464305}
Committed: https://chromium.googlesource.com/chromium/src/+/caa0b215d5d6ce180c4ee1ec8879630e575dba8f
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Total comments: 8
Patch Set 3 : address comments #
Messages
Total messages: 17 (7 generated)
wychen@chromium.org changed reviewers: + haraken@chromium.org
PTAL
haraken@chromium.org changed reviewers: + isherman@chromium.org
+isherman for histograms https://codereview.chromium.org/2815693004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2815693004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:305: // Couldn't use SCOPED_BLINK_UMA_HISTOGRAM_TIMER() due to dynamic naming. Drop this comment. https://codereview.chromium.org/2815693004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:309: extractionHistogram.Count(1e6 * elapsed_time); elapsed_time is in microseconds. So this should be 1e3 * eslapsed_time, right?
https://codereview.chromium.org/2815693004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2815693004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:305: // Couldn't use SCOPED_BLINK_UMA_HISTOGRAM_TIMER() due to dynamic naming. On 2017/04/12 02:10:22, haraken wrote: > > Drop this comment. Done. https://codereview.chromium.org/2815693004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:309: extractionHistogram.Count(1e6 * elapsed_time); On 2017/04/12 02:10:22, haraken wrote: > > elapsed_time is in microseconds. So this should be 1e3 * eslapsed_time, right? > MonotonicallyIncreasingTime() is in seconds.
https://codereview.chromium.org/2815693004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2815693004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:250: // kCount must be the last entry. nit: Please document that this enum is used to back an UMA histogram, and should therefore be treated as append-only. https://codereview.chromium.org/2815693004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:251: enum ExtractionStatus { kOK, kEmpty, kParseFailure, kWrongType, kCount }; nit: Could this be an enum class? https://codereview.chromium.org/2815693004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:307: ("CopylessPaste.ExtractionFailedUs", 1, 1000000, 50)); Optional nit: Could you please write 1000 * 1000 instead of 1000000, for ease of visual parsing? Applies below as well. https://codereview.chromium.org/2815693004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2815693004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9170: + on Android. This only counts pages with successful JSON extraction. Was this always true, or is this a change in semantics? If it's a change in semantics, then please rename the histogram, so that historical data with different semantics is separated off from the new semantics.
https://codereview.chromium.org/2815693004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2815693004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:251: enum ExtractionStatus { kOK, kEmpty, kParseFailure, kWrongType, kCount }; On 2017/04/12 23:25:55, Ilya Sherman wrote: > nit: Could this be an enum class? Great suggestion. The UMA API in base/ supports enum class, but the one in Blink doesn't. static_cast'ing all over doesn't look nice. I heard this would change soon, but I don't want to block this CL. https://codereview.chromium.org/2815693004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2815693004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9170: + on Android. This only counts pages with successful JSON extraction. On 2017/04/12 23:25:55, Ilya Sherman wrote: > Was this always true, or is this a change in semantics? If it's a change in > semantics, then please rename the histogram, so that historical data with > different semantics is separated off from the new semantics. The feature is not launched yet, and we haven't collected any data. In this case, I guess changing the meaning should be fine.
Got it, thanks. In that case, LGTM % my remaining comments.
LGTM
https://codereview.chromium.org/2815693004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp (right): https://codereview.chromium.org/2815693004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:250: // kCount must be the last entry. On 2017/04/12 23:25:55, Ilya Sherman wrote: > nit: Please document that this enum is used to back an UMA histogram, and should > therefore be treated as append-only. Done. https://codereview.chromium.org/2815693004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/document_metadata/CopylessPasteExtractor.cpp:307: ("CopylessPaste.ExtractionFailedUs", 1, 1000000, 50)); On 2017/04/12 23:25:55, Ilya Sherman wrote: > Optional nit: Could you please write 1000 * 1000 instead of 1000000, for ease of > visual parsing? Applies below as well. Done.
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2815693004/#ps40001 (title: "address comments")
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": 1492052620308410,
"parent_rev": "6bfd2af0b9ab88cd377c6902033812556e1d4edd", "commit_rev":
"caa0b215d5d6ce180c4ee1ec8879630e575dba8f"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492052620308410,
"parent_rev": "6bfd2af0b9ab88cd377c6902033812556e1d4edd", "commit_rev":
"caa0b215d5d6ce180c4ee1ec8879630e575dba8f"}
Message was sent while issue was closed.
Description was changed from ========== Add more UMA in CopylessPasteExtractor - Enum of extraction status - Histogram of extraction time (those w/ JSON parsing results are counted separately) BUG=693633 ========== to ========== Add more UMA in CopylessPasteExtractor - Enum of extraction status - Histogram of extraction time (those w/ JSON parsing results are counted separately) BUG=693633 Review-Url: https://codereview.chromium.org/2815693004 Cr-Commit-Position: refs/heads/master@{#464305} Committed: https://chromium.googlesource.com/chromium/src/+/caa0b215d5d6ce180c4ee1ec8879... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/caa0b215d5d6ce180c4ee1ec8879... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
