Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644143006:
Report reasons for a negative histogram sample count. (Closed)
|
Created:
3 years, 11 months ago by bcwhite Modified:
3 years, 11 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReport reasons for a negative histogram sample count.
Samples reported this way will also be prevented from going negative during the upload.
BUG=682680
Review-Url: https://codereview.chromium.org/2644143006
Cr-Commit-Position: refs/heads/master@{#445177}
Committed: https://chromium.googlesource.com/chromium/src/+/48897934e17d9043535dd1213738cb5fce8bda89
Patch Set 1 #
Total comments: 11
Patch Set 2 : addressed review comments by asvitkine #
Messages
Total messages: 22 (15 generated)
The CQ bit was checked by bcwhite@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...
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
Description was changed from ========== Report reasons for a negative histogram sample count. BUG=682680 ========== to ========== Report reasons for a negative histogram sample count. Samples reported this way will also be prevented from going negative during the upload. BUG=682680 ==========
https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... File base/metrics/persistent_sample_map.cc (right): https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... base/metrics/persistent_sample_map.cc:21: NOT_NEGATIVE, Since you're never logging this, suggest just removing it and for the purposes of the placeholder value using MAX_NEGATIVE_SAMPLE_REASONS. https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... base/metrics/persistent_sample_map.cc:214: *bucket = 0; This is also changing behavior - right? Seems it would be better to keep behavior as before (i.e. having the negatives) so that we can debug this, no? Otherwise, it's hard to correlate the new histogram to the actual "bad" data since you're getting rid of it? https://codereview.chromium.org/2644143006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2644143006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:69819: + Reasons why a negative sample count would have been created. This is for sparse histograms specifically, right? If so, mention it in the description.
https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... File base/metrics/persistent_sample_map.cc (right): https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... base/metrics/persistent_sample_map.cc:21: NOT_NEGATIVE, On 2017/01/20 19:11:30, Alexei Svitkine (slow) wrote: > Since you're never logging this, suggest just removing it and for the purposes > of the placeholder value using MAX_NEGATIVE_SAMPLE_REASONS. I use it as a default value in order to be able set the reason through conditions and then check later if that was done. https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... base/metrics/persistent_sample_map.cc:214: *bucket = 0; On 2017/01/20 19:11:30, Alexei Svitkine (slow) wrote: > This is also changing behavior - right? > > Seems it would be better to keep behavior as before (i.e. having the negatives) > so that we can debug this, no? Otherwise, it's hard to correlate the new > histogram to the actual "bad" data since you're getting rid of it? Correct. The benefit this way is that we'll know that any negative samples must come from another source. That seems more reliable than trying to correlate the number of these reports with the number of negative samples. https://codereview.chromium.org/2644143006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2644143006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:69819: + Reasons why a negative sample count would have been created. On 2017/01/20 19:11:30, Alexei Svitkine (slow) wrote: > This is for sparse histograms specifically, right? If so, mention it in the > description. Currently there are only sparse sources but I worded it this way on the assumption that it may be necessary to expands its scope.
lgtm % comments https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... File base/metrics/persistent_sample_map.cc (right): https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... base/metrics/persistent_sample_map.cc:21: NOT_NEGATIVE, On 2017/01/20 19:17:06, bcwhite wrote: > On 2017/01/20 19:11:30, Alexei Svitkine (slow) wrote: > > Since you're never logging this, suggest just removing it and for the purposes > > of the placeholder value using MAX_NEGATIVE_SAMPLE_REASONS. > > I use it as a default value in order to be able set the reason through > conditions and then check later if that was done. Right, I am suggesting you could use MAX_NEGATIVE_SAMPLE_REASONS for the default value in the same way. https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... base/metrics/persistent_sample_map.cc:214: *bucket = 0; On 2017/01/20 19:17:06, bcwhite wrote: > On 2017/01/20 19:11:30, Alexei Svitkine (slow) wrote: > > This is also changing behavior - right? > > > > Seems it would be better to keep behavior as before (i.e. having the > negatives) > > so that we can debug this, no? Otherwise, it's hard to correlate the new > > histogram to the actual "bad" data since you're getting rid of it? > > Correct. The benefit this way is that we'll know that any negative samples must > come from another source. That seems more reliable than trying to correlate the > number of these reports with the number of negative samples. OK. https://codereview.chromium.org/2644143006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2644143006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:69819: + Reasons why a negative sample count would have been created. On 2017/01/20 19:17:06, bcwhite wrote: > On 2017/01/20 19:11:30, Alexei Svitkine (slow) wrote: > > This is for sparse histograms specifically, right? If so, mention it in the > > description. > > Currently there are only sparse sources but I worded it this way on the > assumption that it may be necessary to expands its scope. OK. Can you add a note about it being currently sparse though? The note can be removed/modified in the future if we add more support.
The CQ bit was checked by bcwhite@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...
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
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/2644143006/diff/1/base/metrics/persistent_sam... File base/metrics/persistent_sample_map.cc (right): https://codereview.chromium.org/2644143006/diff/1/base/metrics/persistent_sam... base/metrics/persistent_sample_map.cc:21: NOT_NEGATIVE, On 2017/01/20 19:21:30, Alexei Svitkine (slow) wrote: > On 2017/01/20 19:17:06, bcwhite wrote: > > On 2017/01/20 19:11:30, Alexei Svitkine (slow) wrote: > > > Since you're never logging this, suggest just removing it and for the > purposes > > > of the placeholder value using MAX_NEGATIVE_SAMPLE_REASONS. > > > > I use it as a default value in order to be able set the reason through > > conditions and then check later if that was done. > > Right, I am suggesting you could use MAX_NEGATIVE_SAMPLE_REASONS for the default > value in the same way. Done. https://codereview.chromium.org/2644143006/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2644143006/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:69819: + Reasons why a negative sample count would have been created. On 2017/01/20 19:21:30, Alexei Svitkine (slow) wrote: > On 2017/01/20 19:17:06, bcwhite wrote: > > On 2017/01/20 19:11:30, Alexei Svitkine (slow) wrote: > > > This is for sparse histograms specifically, right? If so, mention it in the > > > description. > > > > Currently there are only sparse sources but I worded it this way on the > > assumption that it may be necessary to expands its scope. > > OK. Can you add a note about it being currently sparse though? The note can be > removed/modified in the future if we add more support. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2644143006/#ps40001 (title: "addressed review comments by asvitkine")
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": 1484948271825050,
"parent_rev": "af0cc1e8e62b43c6db0b857c2abf1da4f99d4ef1", "commit_rev":
"48897934e17d9043535dd1213738cb5fce8bda89"}
Message was sent while issue was closed.
Description was changed from ========== Report reasons for a negative histogram sample count. Samples reported this way will also be prevented from going negative during the upload. BUG=682680 ========== to ========== Report reasons for a negative histogram sample count. Samples reported this way will also be prevented from going negative during the upload. BUG=682680 Review-Url: https://codereview.chromium.org/2644143006 Cr-Commit-Position: refs/heads/master@{#445177} Committed: https://chromium.googlesource.com/chromium/src/+/48897934e17d9043535dd1213738... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/48897934e17d9043535dd1213738... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
