|
|
Created:
4 years, 4 months ago by mattcary Modified:
4 years, 3 months ago CC:
chromium-reviews, shishir+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org, tburkard+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrerender: Remove deprecated prerender experiment histograms.
This only removes the suffixing done for initial prerender experiments using the
prerender mode.
As a side effect, this effectively resets all prerender histograms as all
current histograms currently reported are "_Enabled", and this change drops the
_Enabled suffix and so renames existing histograms. However, all of those
_Enabled histograms seem to be empty currently. Possibly some histograms should
just be removed?
BUG=
Committed: https://crrev.com/81efc02ef67475e403bc5f6c4a79446eda627423
Cr-Commit-Position: refs/heads/master@{#414997}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Drop histogram.xml changes #Patch Set 3 : mor deprecations #
Total comments: 1
Patch Set 4 : don't deprecate all the histograms #
Total comments: 8
Patch Set 5 : trim a few more unsued ones #
Messages
Total messages: 39 (17 generated)
mattcary@chromium.org changed reviewers: + mmenke@chromium.org, pasko@chromium.org
The CQ bit was checked by mattcary@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! https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_histograms.cc:156: "Prerender.OmniboxPrerenderCount", 1, 2); this histogram (Prerender.OmniboxNavigationsUsedPrerenderCount_Enabled) and another one below are actually recorded and query-able in the dashboard, just marked as unowned and obsolete, hence not discoverable. Please update the histograms.xml to say that the _Enabled thing is deprecated. The thing without the suffix is now becoming owned, yay, no modifications required. We have not looked at them before, but let's keep, may be useful. https://codereview.chromium.org/2275953002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2275953002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:90023: -<enum name="PrerenderMode" type="int"> We should not remove anything from histograms.xml because we might still want to look at old data on the dashboard, and the dashboard just uses the latest histograms.xml.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for doing this, LGTM (Modulo all of pasko's comments, which I agree with)
https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_histograms.cc:156: "Prerender.OmniboxPrerenderCount", 1, 2); On 2016/08/24 13:08:29, pasko wrote: > this histogram (Prerender.OmniboxNavigationsUsedPrerenderCount_Enabled) and > another one below are actually recorded and query-able in the dashboard, just > marked as unowned and obsolete, hence not discoverable. > > Please update the histograms.xml to say that the _Enabled thing is deprecated. > The thing without the suffix is now becoming owned, yay, no modifications > required. > > We have not looked at them before, but let's keep, may be useful. Ah, thanks. My uma-foo is weak, I didn't realize that something could exist but not be discoverable. However, there doesn't seem to be any OmniboxPrerenderCount_Enabled in histograms.xml, or any reference to PrerenderMode from OmniboxPrerenderCount. So I don't think there's any further deprecation do be done. In other words, all this CL will do is stop producing the old_* metrics in favor of simpler names (that are already attached to owners). https://codereview.chromium.org/2275953002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2275953002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:90023: -<enum name="PrerenderMode" type="int"> On 2016/08/24 13:08:29, pasko wrote: > We should not remove anything from histograms.xml because we might still want to > look at old data on the dashboard, and the dashboard just uses the latest > histograms.xml. Done.
The CQ bit was checked by mattcary@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_histograms.cc (right): https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_histograms.cc:156: "Prerender.OmniboxPrerenderCount", 1, 2); On 2016/08/24 15:49:11, mattcary wrote: > However, there doesn't seem to be any OmniboxPrerenderCount_Enabled in > histograms.xml, or any reference to PrerenderMode from OmniboxPrerenderCount. So > I don't think there's any further deprecation do be done. > > In other words, all this CL will do is stop producing the old_* metrics in favor > of simpler names (that are already attached to owners). the fact that *_Enabled is not in histograms.xml is a bug, smb did not update it carefully, I wanted to correct it with one small change. On the other hand, these suffixes are messed up and not reflecting what's recorded, and it was this way in 2012 already. Feel free to ignore this suggestion. I think this would be good to fix here, however: * let's announce _all_ of the <histogram_suffixes name="Prerender"> as obsolete, they are incorrect, but as this change lands they'd become even more puzzling. * <histogram_suffixes name="PrerenderSource"> contain these incorrect suffixes as affected-histogram, let's obsolete them too. Example: <affected-histogram name="Prerender.FinalStatus_Prerender5minTTL"/>
On 2016/08/24 17:51:38, pasko wrote: > https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... > File chrome/browser/prerender/prerender_histograms.cc (right): > > https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... > chrome/browser/prerender/prerender_histograms.cc:156: > "Prerender.OmniboxPrerenderCount", 1, 2); > On 2016/08/24 15:49:11, mattcary wrote: > > However, there doesn't seem to be any OmniboxPrerenderCount_Enabled in > > histograms.xml, or any reference to PrerenderMode from OmniboxPrerenderCount. > So > > I don't think there's any further deprecation do be done. > > > > In other words, all this CL will do is stop producing the old_* metrics in > favor > > of simpler names (that are already attached to owners). > > the fact that *_Enabled is not in histograms.xml is a bug, smb did not update it > carefully, I wanted to correct it with one small change. > > On the other hand, these suffixes are messed up and not reflecting what's > recorded, and it was this way in 2012 already. Feel free to ignore this > suggestion. > > > I think this would be good to fix here, however: > > * let's announce _all_ of the <histogram_suffixes name="Prerender"> as obsolete, > they are incorrect, but as this change lands they'd become even more puzzling. > > * <histogram_suffixes name="PrerenderSource"> contain these incorrect suffixes > as affected-histogram, let's obsolete them too. Example: > > <affected-histogram name="Prerender.FinalStatus_Prerender5minTTL"/> Ah, I get you now. Done.
On 2016/08/25 13:43:03, mattcary wrote: > On 2016/08/24 17:51:38, pasko wrote: > > > https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... > > File chrome/browser/prerender/prerender_histograms.cc (right): > > > > > https://codereview.chromium.org/2275953002/diff/1/chrome/browser/prerender/pr... > > chrome/browser/prerender/prerender_histograms.cc:156: > > "Prerender.OmniboxPrerenderCount", 1, 2); > > On 2016/08/24 15:49:11, mattcary wrote: > > > However, there doesn't seem to be any OmniboxPrerenderCount_Enabled in > > > histograms.xml, or any reference to PrerenderMode from > OmniboxPrerenderCount. > > So > > > I don't think there's any further deprecation do be done. > > > > > > In other words, all this CL will do is stop producing the old_* metrics in > > favor > > > of simpler names (that are already attached to owners). > > > > the fact that *_Enabled is not in histograms.xml is a bug, smb did not update > it > > carefully, I wanted to correct it with one small change. > > > > On the other hand, these suffixes are messed up and not reflecting what's > > recorded, and it was this way in 2012 already. Feel free to ignore this > > suggestion. > > > > > > I think this would be good to fix here, however: > > > > * let's announce _all_ of the <histogram_suffixes name="Prerender"> as > obsolete, > > they are incorrect, but as this change lands they'd become even more puzzling. > > > > * <histogram_suffixes name="PrerenderSource"> contain these incorrect suffixes > > as affected-histogram, let's obsolete them too. Example: > > > > <affected-histogram name="Prerender.FinalStatus_Prerender5minTTL"/> > > Ah, I get you now. Done. At least, tried. I don't fully understand the histogram system yet.
https://codereview.chromium.org/2275953002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2275953002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102573: + <obsolete> some of the below is still not obsolete :) the suffixes (with ordering="prefix", ha ha) are obsolete only for the affected-histogram parts where <affected-histogram ...> has _Prerender{Enabled,Control,5minTTL,...}. Discussed offline, decided to move them to a separate <histogram_suffixes name="DeprecatedPrerenderSource">
Okay try #2 of X. I split out the deprecated parts of PrerenderSource to DeprecatedPrerenderSource. LMK
https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102631: <affected-histogram name="Prerender.FinalStatusMatchComplete"/> this guy is also deprecated, and I'm personally guilty for not removing it from here https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102642: <affected-histogram name="Prerender.FractionPixelsFinalAtSwapin"/> not recorded https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102655: <affected-histogram name="Prerender.LocalPredictorEvent"/> LocalPredictor is long gone as well https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102749: <affected-histogram name="Prerender.PerceivedPLTMatchedComplete"/> gone
on the other hand, these remaining deprecated-but-not-marked-as-obsolete are not very relevant to this specific change, we can clean them up later so LGTM, thank you for cleanup!
On 2016/08/25 17:21:33, pasko wrote: > on the other hand, these remaining deprecated-but-not-marked-as-obsolete are not > very relevant to this specific change, we can clean them up later > > so LGTM, thank you for cleanup! I went ahead and did your suggestions and called it a day.
https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102631: <affected-histogram name="Prerender.FinalStatusMatchComplete"/> On 2016/08/25 16:32:04, pasko wrote: > this guy is also deprecated, and I'm personally guilty for not removing it from > here Done. https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102642: <affected-histogram name="Prerender.FractionPixelsFinalAtSwapin"/> On 2016/08/25 16:32:04, pasko wrote: > not recorded Done. https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102655: <affected-histogram name="Prerender.LocalPredictorEvent"/> On 2016/08/25 16:32:04, pasko wrote: > LocalPredictor is long gone as well Done. https://codereview.chromium.org/2275953002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:102749: <affected-histogram name="Prerender.PerceivedPLTMatchedComplete"/> On 2016/08/25 16:32:04, pasko wrote: > gone Done.
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2275953002/#ps80001 (title: "trim a few more unsued ones")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mattcary@chromium.org changed reviewers: + isherman@chromium.org
+isherman for histograms.xml review. Thanks!
Thanks for the cleanup! LGTM.
The CQ bit was checked by mattcary@chromium.org
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
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_...)
The CQ bit was checked by mattcary@chromium.org
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 #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Prerender: Remove deprecated prerender experiment histograms. This only removes the suffixing done for initial prerender experiments using the prerender mode. As a side effect, this effectively resets all prerender histograms as all current histograms currently reported are "_Enabled", and this change drops the _Enabled suffix and so renames existing histograms. However, all of those _Enabled histograms seem to be empty currently. Possibly some histograms should just be removed? BUG= ========== to ========== Prerender: Remove deprecated prerender experiment histograms. This only removes the suffixing done for initial prerender experiments using the prerender mode. As a side effect, this effectively resets all prerender histograms as all current histograms currently reported are "_Enabled", and this change drops the _Enabled suffix and so renames existing histograms. However, all of those _Enabled histograms seem to be empty currently. Possibly some histograms should just be removed? BUG= Committed: https://crrev.com/81efc02ef67475e403bc5f6c4a79446eda627423 Cr-Commit-Position: refs/heads/master@{#414997} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/81efc02ef67475e403bc5f6c4a79446eda627423 Cr-Commit-Position: refs/heads/master@{#414997} |