Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2966773002:
Add histograms to learn about metrics files dropped for lack of an embedded profile. (Closed)
|
Created:
3 years, 5 months ago by bcwhite Modified:
3 years, 5 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histograms to learn about metrics files dropped for lack of an embedded profile.
BUG=695880
Review-Url: https://codereview.chromium.org/2966773002
Cr-Commit-Position: refs/heads/master@{#483746}
Committed: https://chromium.googlesource.com/chromium/src/+/757034c63af4b1c61eeebf070e2123f787be51f7
Patch Set 1 #
Total comments: 6
Patch Set 2 : addressed review comments by asvitkine #
Total comments: 2
Patch Set 3 : only 50 bucktes for file age #
Messages
Total messages: 23 (16 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
https://codereview.chromium.org/2966773002/diff/1/components/metrics/file_met... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2966773002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:561: } Nit: How about: while (histogram_iter.GetNext()) { ++histogram_count; } https://codereview.chromium.org/2966773002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:562: UMA_HISTOGRAM_COUNTS_1000( Nit: Suggest using 10k version of the macro because I *think* there could be more than 1000. https://codereview.chromium.org/2966773002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:571: base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(1), 100); I think logging in ms is not useful here, so you don't need to use CUSTOM_TIMES. Also 1 day seems like too low a cap. Suggest minute granularity, 50 buckets and let's say 30 days as max. i.e. similar to what we do for seed freshness: UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedFreshness", delta.InMinutes(), 1, base::TimeDelta::FromDays(30).InMinutes(), 50);
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 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/2966773002/diff/1/components/metrics/file_met... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2966773002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:561: } On 2017/06/30 14:50:10, Alexei Svitkine (slow) wrote: > Nit: How about: > > while (histogram_iter.GetNext()) { > ++histogram_count; > } Done. https://codereview.chromium.org/2966773002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:562: UMA_HISTOGRAM_COUNTS_1000( On 2017/06/30 14:50:10, Alexei Svitkine (slow) wrote: > Nit: Suggest using 10k version of the macro because I *think* there could be > more than 1000. Done. It doesn't really matter either way. If it's more than 1000, you know you have a full run and not an early crash that just didn't get around to writing a system profile. https://codereview.chromium.org/2966773002/diff/1/components/metrics/file_met... components/metrics/file_metrics_provider.cc:571: base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(1), 100); On 2017/06/30 14:50:10, Alexei Svitkine (slow) wrote: > I think logging in ms is not useful here, so you don't need to use CUSTOM_TIMES. > > Also 1 day seems like too low a cap. > > Suggest minute granularity, 50 buckets and let's say 30 days as max. > > i.e. similar to what we do for seed freshness: > > UMA_HISTOGRAM_CUSTOM_COUNTS("Variations.SeedFreshness", delta.InMinutes(), > 1, base::TimeDelta::FromDays(30).InMinutes(), 50); Done.
lgtm https://codereview.chromium.org/2966773002/diff/20001/components/metrics/file... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2966773002/diff/20001/components/metrics/file... components/metrics/file_metrics_provider.cc:567: base::TimeDelta::FromDays(30).InMinutes(), 100); Nit: Suggest 50 buckets. I don't think we need 100 bucket granularity.
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 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/2966773002/diff/20001/components/metrics/file... File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/2966773002/diff/20001/components/metrics/file... components/metrics/file_metrics_provider.cc:567: base::TimeDelta::FromDays(30).InMinutes(), 100); On 2017/06/30 16:28:40, Alexei Svitkine (slow) wrote: > Nit: Suggest 50 buckets. I don't think we need 100 bucket granularity. Done.
The CQ bit was unchecked by bcwhite@chromium.org
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/2966773002/#ps40001 (title: "only 50 bucktes for file age")
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": 1498840473210650,
"parent_rev": "cd009ff3bf0b8c024f7d3af84ad82af370ff98b1", "commit_rev":
"757034c63af4b1c61eeebf070e2123f787be51f7"}
Message was sent while issue was closed.
Description was changed from ========== Add histograms to learn about metrics files dropped for lack of an embedded profile. BUG=695880 ========== to ========== Add histograms to learn about metrics files dropped for lack of an embedded profile. BUG=695880 Review-Url: https://codereview.chromium.org/2966773002 Cr-Commit-Position: refs/heads/master@{#483746} Committed: https://chromium.googlesource.com/chromium/src/+/757034c63af4b1c61eeebf070e21... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/757034c63af4b1c61eeebf070e21... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
