Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2226083002:
Remove a number of non-user triggered UMA actions. (Closed)
|
Created:
4 years, 4 months ago by Alexei Svitkine (slow) Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, groby-ooo-7-16 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove a number of non-user triggered UMA actions.
Specifically, removes the following UMA actions:
FrameLoad, FrameLoadWithFlash,
The non-suffixed version would be logged by every page
load (including pages that use a script to auto reload
themselves) and is thus pretty spammy.
It also seems like the kind of info they were tracking
would be better suited to track with enum histograms.
These could be added if there's a desire to keep these
stats.
BUG=624504
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/e2be21dbfe73a8e41cd980504567b002d4e03222
Cr-Commit-Position: refs/heads/master@{#411339}
Patch Set 1 #Patch Set 2 : only remove FrameLoad #Patch Set 3 : Add mpearson to NavEntryCommitted owners #
Messages
Total messages: 29 (11 generated)
Description was changed from ========== Remove a number of non-user triggered UMA actions. Specifically, removes the following UMA actions: FrameLoad, FrameLoadWithFlash, NavEntryCommitted, NavEntryCommitted.SRP The non-suffixed versions of these would be logged by every page load (including pages that use a script to auto reload themselves) and are thus pretty spammy. It also seems like the kind of info they were tracking would be better suited to track with enum histograms. These could be added if there's a desire to keep these stats. BUG=624504 ========== to ========== Remove a number of non-user triggered UMA actions. Specifically, removes the following UMA actions: FrameLoad, FrameLoadWithFlash, NavEntryCommitted, NavEntryCommitted.SRP The non-suffixed versions of these would be logged by every page load (including pages that use a script to auto reload themselves) and are thus pretty spammy. It also seems like the kind of info they were tracking would be better suited to track with enum histograms. These could be added if there's a desire to keep these stats. BUG=624504 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by asvitkine@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...
asvitkine@chromium.org changed reviewers: + mpearson@chromium.org, thestig@chromium.org
mpearson & thestig: please review thestig: You're the owner of FrameLoad and FrameLoadWithFlash - let me know if these are still useful. If they are, maybe they can be converted to a histogram? mpearson: Looks like NavEntryCommitted.SRP was added for InstantExtended back in the day by rlp@ who no longer works on Chrome. Do you know if anyone might still be using it? Happy to convert either/both to a histogram, if we still want them. Of course, if we're not using the data at all, we should just delete as this CL is doing. Let me know what you think!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
NavEntryCommitted is user-triggered, just like PageLoad. I tend to use NavEntryCommitted instead of PageLoad because it's less spammy, more indicative of actual user-initiated loads than PageLoad. (If you keep chrome://user-actions open while you do some things on the web, you'll regularly see additional page loads recorded that--at least to me--don't feel like I did anything that merits another page load being emitted.) --mark
Not sure why I set up myself as the sole owner. +tommycli +groby - do we still want the Flash metric?
On 2016/08/08 22:14:24, Lei Zhang wrote: > Not sure why I set up myself as the sole owner. +tommycli +groby - do we still > want the Flash metric? I think the Plugin.FlashUsage histogram covers the same ground as FrameLoad and FrameLoadWithFlash in a better way. I don't think anyone has used FrameLoadWithFlash in some time.
On 2016/08/08 22:12:01, Mark P wrote: > NavEntryCommitted is user-triggered, just like PageLoad. > > I tend to use NavEntryCommitted instead of PageLoad because it's less spammy, > more indicative of actual user-initiated loads than PageLoad. (If you keep > chrome://user-actions open while you do some things on the web, you'll regularly > see additional page loads recorded that--at least to me--don't feel like I did > anything that merits another page load being emitted.) > > --mark Well, it's actually not user triggered in the sense that a page that auto refreshes itself via JS will trigger it. The linked bug has an example such page. I can totally buy the argument that these are still useful for now - but if we want to have something of the sort that's actually use triggered, it sounds like we need to fix the metrics so that what we use is guaranteed to be user triggered. Anyway, happy to keep this one if you think it's still useful. I can scope down this CL to just removing the FrameLoad* ones. Let me know what you think.
On 2016/08/08 22:37:32, Alexei Svitkine (very slow) wrote: > On 2016/08/08 22:12:01, Mark P wrote: > > NavEntryCommitted is user-triggered, just like PageLoad. > > > > I tend to use NavEntryCommitted instead of PageLoad because it's less spammy, > > more indicative of actual user-initiated loads than PageLoad. (If you keep > > chrome://user-actions open while you do some things on the web, you'll > regularly > > see additional page loads recorded that--at least to me--don't feel like I did > > anything that merits another page load being emitted.) > > > > --mark > > Well, it's actually not user triggered in the sense that a page that auto > refreshes itself via JS will trigger it. The linked bug has an example such > page. Acknowledged. It's not perfect but it's the best (in my opinion) page-load-like metric we have right now. > I can totally buy the argument that these are still useful for now - but if we > want to have something of the sort that's actually use triggered, it sounds like > we need to fix the metrics so that what we use is guaranteed to be user > triggered. Agreed, though not sure if it's worth the trouble. > Anyway, happy to keep this one if you think it's still useful. I can scope down > this CL to just removing the FrameLoad* ones. Let me know what you think. Sounds good to me. --mark
Description was changed from ========== Remove a number of non-user triggered UMA actions. Specifically, removes the following UMA actions: FrameLoad, FrameLoadWithFlash, NavEntryCommitted, NavEntryCommitted.SRP The non-suffixed versions of these would be logged by every page load (including pages that use a script to auto reload themselves) and are thus pretty spammy. It also seems like the kind of info they were tracking would be better suited to track with enum histograms. These could be added if there's a desire to keep these stats. BUG=624504 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove a number of non-user triggered UMA actions. Specifically, removes the following UMA actions: FrameLoad, FrameLoadWithFlash, The non-suffixed version would be logged by every page load (including pages that use a script to auto reload themselves) and is thus pretty spammy. It also seems like the kind of info they were tracking would be better suited to track with enum histograms. These could be added if there's a desire to keep these stats. BUG=624504 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Changed to just obsolete the FrameLoad ones. PTAL.
By the way, NavEntryCommitted actions are unowned. Mark, do you want to own them, given no one else is?
On 2016/08/09 01:15:49, Alexei Svitkine (very slow) wrote: > By the way, NavEntryCommitted actions are unowned. Mark, do you want to own > them, given no one else is? Sure. --mark
Added you as an owner for those. PTAL.
lgtm
lgtm, but you may still need more OWNERS.
asvitkine@chromium.org changed reviewers: + avi@chromium.org
+avi for content/ OWNERS
avi: friendly ping for content/ owners
lgtm stampity stamp
The CQ bit was checked by asvitkine@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.
Description was changed from ========== Remove a number of non-user triggered UMA actions. Specifically, removes the following UMA actions: FrameLoad, FrameLoadWithFlash, The non-suffixed version would be logged by every page load (including pages that use a script to auto reload themselves) and is thus pretty spammy. It also seems like the kind of info they were tracking would be better suited to track with enum histograms. These could be added if there's a desire to keep these stats. BUG=624504 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove a number of non-user triggered UMA actions. Specifically, removes the following UMA actions: FrameLoad, FrameLoadWithFlash, The non-suffixed version would be logged by every page load (including pages that use a script to auto reload themselves) and is thus pretty spammy. It also seems like the kind of info they were tracking would be better suited to track with enum histograms. These could be added if there's a desire to keep these stats. BUG=624504 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove a number of non-user triggered UMA actions. Specifically, removes the following UMA actions: FrameLoad, FrameLoadWithFlash, The non-suffixed version would be logged by every page load (including pages that use a script to auto reload themselves) and is thus pretty spammy. It also seems like the kind of info they were tracking would be better suited to track with enum histograms. These could be added if there's a desire to keep these stats. BUG=624504 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove a number of non-user triggered UMA actions. Specifically, removes the following UMA actions: FrameLoad, FrameLoadWithFlash, The non-suffixed version would be logged by every page load (including pages that use a script to auto reload themselves) and is thus pretty spammy. It also seems like the kind of info they were tracking would be better suited to track with enum histograms. These could be added if there's a desire to keep these stats. BUG=624504 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/e2be21dbfe73a8e41cd980504567b002d4e03222 Cr-Commit-Position: refs/heads/master@{#411339} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e2be21dbfe73a8e41cd980504567b002d4e03222 Cr-Commit-Position: refs/heads/master@{#411339} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
