Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2296543002:
Quantify initial stability report edge cases. (Closed)
|
Created:
4 years, 3 months ago by manzagop (departed) Modified:
4 years, 3 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionQuantify initial stability report edge cases.
BUG=415982
Committed: https://crrev.com/ea99d195854d89efe7bebb318c2448cb94f0befc
Cr-Commit-Position: refs/heads/master@{#417433}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Merge #Patch Set 3 : Switch to histograms #Patch Set 4 : Drop stray comment #Patch Set 5 : Fix unittests #
Total comments: 15
Patch Set 6 : Address comments #
Total comments: 4
Patch Set 7 : Address patch 6 comments #
Total comments: 2
Patch Set 8 : Revise horizontal whitespace in histogram_macros.h #
Total comments: 2
Patch Set 9 : Change a metric's name #
Messages
Total messages: 40 (24 generated)
manzagop@chromium.org changed reviewers: + asvitkine@chromium.org
Hi asvitkine@, I still need to polish/actually try it, but sending early to make sure I'm going in the right direction. Thanks, Pierre
https://codereview.chromium.org/2296543002/diff/1/components/metrics/metrics_... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/2296543002/diff/1/components/metrics/metrics_... components/metrics/metrics_log.cc:221: if (incomplete_shutdown_count) Maybe break this out in a separate change? Seems useful on its own while we continue discussing the other parts. (This is addressing the TODO(jar) right? Remove the TODO then?) https://codereview.chromium.org/2296543002/diff/1/components/metrics/proto/sy... File components/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/2296543002/diff/1/components/metrics/proto/sy... components/metrics/proto/system_profile.proto:562: optional int32 version_mismatch_count = 30; I'd actually prefer these be added as histograms. Histograms have a number of advantages: not needing any server-side changes, automatically useable from the various dashboards we have, less "set in stone" - so that can be added/removed "easier". You can still have the prefs that persist the values between sessions, but the actual values can be logged as histograms.
The CQ bit was checked by manzagop@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by manzagop@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...
I've switched to histograms. Please have another look! https://chromiumcodereview.appspot.com/2296543002/diff/1/components/metrics/m... File components/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/2296543002/diff/1/components/metrics/m... components/metrics/metrics_log.cc:221: if (incomplete_shutdown_count) On 2016/08/30 21:25:59, Alexei Svitkine wrote: > Maybe break this out in a separate change? Seems useful on its own while we > continue discussing the other parts. (This is addressing the TODO(jar) right? > Remove the TODO then?) Done. http://crrev.com/2294323002 https://chromiumcodereview.appspot.com/2296543002/diff/1/components/metrics/p... File components/metrics/proto/system_profile.proto (right): https://chromiumcodereview.appspot.com/2296543002/diff/1/components/metrics/p... components/metrics/proto/system_profile.proto:562: optional int32 version_mismatch_count = 30; On 2016/08/30 21:25:59, Alexei Svitkine (slow) wrote: > I'd actually prefer these be added as histograms. > > Histograms have a number of advantages: not needing any server-side changes, > automatically useable from the various dashboards we have, less "set in stone" - > so that can be added/removed "easier". > > You can still have the prefs that persist the values between sessions, but the > actual values can be logged as histograms. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... File components/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_log.cc:254: UMA_HISTOGRAM_COUNTS_100("Stability.InitialStabilityLog.DeferredCount", If you want these to be logged in the initial stability log, they need to be logged with the Stability flag. This flag makes it so such histograms are added to the initial stability log (as it ignores all other histograms). We have UMA_STABILITY_HISTOGRAM_ENUMERATION() macro, but don't have UMA_STABILITY_HISTOGRAM_COUNTS_100(). Feel free to add one. https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_log.cc:261: UMA_HISTOGRAM_COUNTS_100("Stability.InitialStabilityLog.DiscardCount", Will these always be logged just in the initial stability log? What guarantees that? (i.e. I don't see a check for f (log_type() == INITIAL_STABILITY_LOG)). https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... File components/metrics/metrics_log.h (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_log.h:105: bool LoadSavedEnvironmentFromPrefs(std::string* app_version); Nit: Document the param in the comment. https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... File components/metrics/metrics_service.cc (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_service.cc:932: &system_profile_app_version)) Nit: Add {}'s https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... File components/metrics/metrics_service.h (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_service.h:357: bool PrepareInitialStabilityLog(const std::string& prefs_previous_version); Nit: Document this parameter in the comment. https://chromiumcodereview.appspot.com/2296543002/diff/80001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:57426: +<histogram name="Stability.InitialStabilityLog.DeferredCount"> Nit: Add units= for all of these. Even if it's just units="counts". https://chromiumcodereview.appspot.com/2296543002/diff/80001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:57430: + startup. Nit: For all of these, mention when this is logged.
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by manzagop@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...
Addressed comments! Please have another look. https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... File components/metrics/metrics_log.cc (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_log.cc:254: UMA_HISTOGRAM_COUNTS_100("Stability.InitialStabilityLog.DeferredCount", On 2016/09/02 17:57:49, Alexei Svitkine wrote: > If you want these to be logged in the initial stability log, they need to be > logged with the Stability flag. This flag makes it so such histograms are added > to the initial stability log (as it ignores all other histograms). > > We have UMA_STABILITY_HISTOGRAM_ENUMERATION() macro, but don't have > UMA_STABILITY_HISTOGRAM_COUNTS_100(). Feel free to add one. Oh, good to know! Done. https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_log.cc:261: UMA_HISTOGRAM_COUNTS_100("Stability.InitialStabilityLog.DiscardCount", On 2016/09/02 17:57:49, Alexei Svitkine wrote: > Will these always be logged just in the initial stability log? What guarantees > that? (i.e. I don't see a check for f (log_type() == INITIAL_STABILITY_LOG)). No, they might be logged with any log. My thinking is if there is no initial log, this enables faster reporting. The discard count is special in that it's not cleared on upgrade, so a given version's data will also include discards of past versions, but I think that's better if we want an idea of how much is lost. If we cared, we could have a second histogram that is version specific, and gets cleared on upgrade. https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... File components/metrics/metrics_log.h (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_log.h:105: bool LoadSavedEnvironmentFromPrefs(std::string* app_version); On 2016/09/02 17:57:49, Alexei Svitkine wrote: > Nit: Document the param in the comment. Done. https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... File components/metrics/metrics_service.cc (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_service.cc:932: &system_profile_app_version)) On 2016/09/02 17:57:49, Alexei Svitkine wrote: > Nit: Add {}'s Done. https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... File components/metrics/metrics_service.h (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/components/metri... components/metrics/metrics_service.h:357: bool PrepareInitialStabilityLog(const std::string& prefs_previous_version); On 2016/09/02 17:57:49, Alexei Svitkine wrote: > Nit: Document this parameter in the comment. Done. https://chromiumcodereview.appspot.com/2296543002/diff/80001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/2296543002/diff/80001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:57426: +<histogram name="Stability.InitialStabilityLog.DeferredCount"> On 2016/09/02 17:57:49, Alexei Svitkine wrote: > Nit: Add units= for all of these. Even if it's just units="counts". Done. https://chromiumcodereview.appspot.com/2296543002/diff/80001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:57430: + startup. On 2016/09/02 17:57:49, Alexei Svitkine wrote: > Nit: For all of these, mention when this is logged. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Sorry for the delay. LGTM % comments. https://codereview.chromium.org/2296543002/diff/80001/components/metrics/metr... File components/metrics/metrics_log.cc (right): https://codereview.chromium.org/2296543002/diff/80001/components/metrics/metr... components/metrics/metrics_log.cc:261: UMA_HISTOGRAM_COUNTS_100("Stability.InitialStabilityLog.DiscardCount", On 2016/09/02 21:00:05, manzagop wrote: > On 2016/09/02 17:57:49, Alexei Svitkine wrote: > > Will these always be logged just in the initial stability log? What guarantees > > that? (i.e. I don't see a check for f (log_type() == INITIAL_STABILITY_LOG)). > > No, they might be logged with any log. My thinking is if there is no initial > log, this enables faster reporting. > > The discard count is special in that it's not cleared on upgrade, so a given > version's data will also include discards of past versions, but I think that's > better if we want an idea of how much is lost. If we cared, we could have a > second histogram that is version specific, and gets cleared on upgrade. Ah, I see - the InitialStabilityLog refers to "what" they're talking about, not where they're logged. https://codereview.chromium.org/2296543002/diff/120001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2296543002/diff/120001/base/metrics/histogram... base/metrics/histogram_macros.h:245: #define UMA_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(name, sample, min, max, \ Nit: Don't use UMA_ prefix here since the kUma flag is not necessarily set. I would actually name the INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG to indicate it's not meant to be used from outside this file. Could you also add the INTERNAL_ prefix to HISTOGRAM_ENUMERATION_WITH_FLAG macro for consistency? Finally, can you define this somewhere above - so it's defined before it's used. (It's not strictly necessary for correct compilation, but I found it weird to use before it's been declared.) https://codereview.chromium.org/2296543002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2296543002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57578: + Number of times stability data was discarded. This is accumulated since the This is not actually about the initial stability log. So the name of the metric is kind of confusing. How about instead of using InitialStabilityLog as the substring in the name, instead doing something like "Internals" i.e. Stability.Internals.DataDiscardCount And for the one above Stability.Internals.InitialStabilityLogDeferredCount
The CQ bit was checked by manzagop@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...
Addressed commments. Might be worth another quick look, to be on the safe side (esp. the macros). https://codereview.chromium.org/2296543002/diff/120001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2296543002/diff/120001/base/metrics/histogram... base/metrics/histogram_macros.h:245: #define UMA_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(name, sample, min, max, \ On 2016/09/08 18:41:12, Alexei Svitkine (very slow) wrote: > Nit: Don't use UMA_ prefix here since the kUma flag is not necessarily set. > > I would actually name the INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG to indicate > it's not meant to be used from outside this file. Could you also add the > INTERNAL_ prefix to HISTOGRAM_ENUMERATION_WITH_FLAG macro for consistency? > > Finally, can you define this somewhere above - so it's defined before it's used. > (It's not strictly necessary for correct compilation, but I found it weird to > use before it's been declared.) Done. Also rewrote LOCAL_HISTOGRAM_CUSTOM_COUNTS to make use of it. Tell me if the new location is ok. I've moved it close to INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG. I've put it after LOCAL_HISTOGRAM_CUSTOM_COUNTS to follow with the trend, since LOCAL_HISTOGRAM_CUSTOM_COUNTS is defined after eg LOCAL_HISTOGRAM_COUNTS_100 which makes use of it. https://codereview.chromium.org/2296543002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2296543002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57578: + Number of times stability data was discarded. This is accumulated since the On 2016/09/08 18:41:12, Alexei Svitkine (very slow) wrote: > This is not actually about the initial stability log. So the name of the metric > is kind of confusing. > > How about instead of using InitialStabilityLog as the substring in the name, > instead doing something like "Internals" > > i.e. Stability.Internals.DataDiscardCount > > And for the one above Stability.Internals.InitialStabilityLogDeferredCount Done.
still LGTM % comment https://codereview.chromium.org/2296543002/diff/140001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2296543002/diff/140001/base/metrics/histogram... base/metrics/histogram_macros.h:143: INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG( \ Nit: Your indents seem off. Should be indented 4 and then 4 more on next line. Please fix throughout.
The CQ bit was checked by manzagop@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...
Thanks! Did you want to give a final peek? https://codereview.chromium.org/2296543002/diff/140001/base/metrics/histogram... File base/metrics/histogram_macros.h (right): https://codereview.chromium.org/2296543002/diff/140001/base/metrics/histogram... base/metrics/histogram_macros.h:143: INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG( \ On 2016/09/08 21:00:41, Alexei Svitkine (very slow) wrote: > Nit: Your indents seem off. Should be indented 4 and then 4 more on next line. > Please fix throughout. 'git cl format' is what gives the 2 space indent... I'm not sure the style guide says something about line continuation. In any case, consistency is the way to go. :) Done.
lgtm % comment https://codereview.chromium.org/2296543002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2296543002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57746: +<histogram name="Stability.InitialStabilityLog.VersionMismatchCount" Sorry, I should have been more explicit - but meant that you should use Stability.Internals naming here too.
The CQ bit was checked by manzagop@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...
Converging! Another look? https://codereview.chromium.org/2296543002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2296543002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57746: +<histogram name="Stability.InitialStabilityLog.VersionMismatchCount" On 2016/09/08 21:25:55, Alexei Svitkine (very slow) wrote: > Sorry, I should have been more explicit - but meant that you should use > Stability.Internals naming here too. Ah sorry, I wasn't sure and should have checked. Done.
lgtm
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 manzagop@chromium.org
Thanks for the review!
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.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Quantify initial stability report edge cases. BUG=415982 ========== to ========== Quantify initial stability report edge cases. BUG=415982 Committed: https://crrev.com/ea99d195854d89efe7bebb318c2448cb94f0befc Cr-Commit-Position: refs/heads/master@{#417433} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ea99d195854d89efe7bebb318c2448cb94f0befc Cr-Commit-Position: refs/heads/master@{#417433} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
