Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2426473006:
Add support for base histograms in histograms.xml. (Closed)
|
Created:
4 years, 2 months ago by Bryan McQuade Modified:
4 years, 2 months ago Reviewers:
Ilya Sherman CC:
asvitkine+watch_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for base histograms in histograms.xml.
A base histogram is a histogram that is intended to be suffixed,
but that shouldn't itself show up in the histogram UI. A base
histogram is conceptually similar to an abstract base class.
The histograms.xml syntax is:
<histogram base="true" name="PageLoad.AbortTiming.Background" units="ms">
A similar syntax is added for marking suffixes as 'base':
<suffix base="true" name="AfterPaint.BeforeInteraction"/>
PageLoad.AbortTiming base histograms do not record samples, so we
want to avoid showing them in the UI. To do so, we mark them
as base histograms using the new 'base' histogram attribute.
For now, 'base' histograms are simply marked as 'obsolete' with a
default obsolete reason 'Base histogram. Use suffixes of this
histogram instead.'. In the future, we can consider plumbing the
'base' information through to the UI using a mechanism other than
the 'obsolete' reason, if a reason to do so emerges.
BUG=656119
Committed: https://crrev.com/e3fc97108d47f023e6843a15210a7a956c6e18d7
Cr-Commit-Position: refs/heads/master@{#426087}
Patch Set 1 #Patch Set 2 : update syntax #
Total comments: 2
Patch Set 3 : address comment #
Messages
Total messages: 25 (19 generated)
Description was changed from ========== Add support for marking base histograms as obsolete/hidden. PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as obsolete. Prior to this patch, obsoletedness was inherited by suffixed histograms. With this patch, we need to mark a base histogram as obsolete, but allow suffix histograms to be non-obsolete. Thus, we add a new 'inherit' attribute on the <obsolete> tag which allows base histograms to specify whether their child histograms should inherit their obsolete status, like so <obsolete inherit="false"> obsolete reason goes here. </obsolete> BUG= ========== to ========== Add support to histograms.xml for base histograms. PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. BUG= ==========
The CQ bit was checked by bmcquade@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...
Description was changed from ========== Add support to histograms.xml for base histograms. PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. BUG= ========== to ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason. BUG= ==========
Description was changed from ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason. BUG= ========== to ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason. BUG=656119 ==========
Description was changed from ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason. BUG=656119 ========== to ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason, if a reason to do so emerges. BUG=656119 ==========
bmcquade@chromium.org changed reviewers: + isherman@chromium.org
Description was changed from ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason, if a reason to do so emerges. BUG=656119 ========== to ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. The histograms.xml syntax is: <histogram base="true" name="PageLoad.AbortTiming.Background" units="ms"> A similar syntax exists for making suffixes as 'base': <suffix base="true" name="AfterPaint.BeforeInteraction"/> PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason, if a reason to do so emerges. BUG=656119 ==========
Description was changed from ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. The histograms.xml syntax is: <histogram base="true" name="PageLoad.AbortTiming.Background" units="ms"> A similar syntax exists for making suffixes as 'base': <suffix base="true" name="AfterPaint.BeforeInteraction"/> PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason, if a reason to do so emerges. BUG=656119 ========== to ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. The histograms.xml syntax is: <histogram base="true" name="PageLoad.AbortTiming.Background" units="ms"> A similar syntax is added for marking suffixes as 'base': <suffix base="true" name="AfterPaint.BeforeInteraction"/> PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason, if a reason to do so emerges. BUG=656119 ==========
Ilya, here's a change to implement my understanding of what you proposed in your comments in https://codereview.chromium.org/2424503002. Please take a look and let me know what you think. Thanks!
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_...)
Description was changed from ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. The histograms.xml syntax is: <histogram base="true" name="PageLoad.AbortTiming.Background" units="ms"> A similar syntax is added for marking suffixes as 'base': <suffix base="true" name="AfterPaint.BeforeInteraction"/> PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark the base histograms as such using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as obsolete with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason, if a reason to do so emerges. BUG=656119 ========== to ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. The histograms.xml syntax is: <histogram base="true" name="PageLoad.AbortTiming.Background" units="ms"> A similar syntax is added for marking suffixes as 'base': <suffix base="true" name="AfterPaint.BeforeInteraction"/> PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark them as base histograms using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as 'obsolete' with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason, if a reason to do so emerges. BUG=656119 ==========
LGTM. Thanks for implementing this, Bryan! =) https://codereview.chromium.org/2426473006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/extract_histograms.py (right): https://codereview.chromium.org/2426473006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/extract_histograms.py:417: # histogram must explicitly declare themselves as base histograms. nit: s/histogram/histograms
The CQ bit was checked by bmcquade@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/2426473006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/extract_histograms.py (right): https://codereview.chromium.org/2426473006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/extract_histograms.py:417: # histogram must explicitly declare themselves as base histograms. On 2016/10/18 at 21:05:58, Ilya Sherman wrote: > nit: s/histogram/histograms done, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2426473006/#ps40001 (title: "address comment")
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. The histograms.xml syntax is: <histogram base="true" name="PageLoad.AbortTiming.Background" units="ms"> A similar syntax is added for marking suffixes as 'base': <suffix base="true" name="AfterPaint.BeforeInteraction"/> PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark them as base histograms using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as 'obsolete' with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason, if a reason to do so emerges. BUG=656119 ========== to ========== Add support for base histograms in histograms.xml. A base histogram is a histogram that is intended to be suffixed, but that shouldn't itself show up in the histogram UI. A base histogram is conceptually similar to an abstract base class. The histograms.xml syntax is: <histogram base="true" name="PageLoad.AbortTiming.Background" units="ms"> A similar syntax is added for marking suffixes as 'base': <suffix base="true" name="AfterPaint.BeforeInteraction"/> PageLoad.AbortTiming base histograms do not record samples, so we want to avoid showing them in the UI. To do so, we mark them as base histograms using the new 'base' histogram attribute. For now, 'base' histograms are simply marked as 'obsolete' with a default obsolete reason 'Base histogram. Use suffixes of this histogram instead.'. In the future, we can consider plumbing the 'base' information through to the UI using a mechanism other than the 'obsolete' reason, if a reason to do so emerges. BUG=656119 Committed: https://crrev.com/e3fc97108d47f023e6843a15210a7a956c6e18d7 Cr-Commit-Position: refs/heads/master@{#426087} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e3fc97108d47f023e6843a15210a7a956c6e18d7 Cr-Commit-Position: refs/heads/master@{#426087} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
