New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 894168 link

Starred by 2 users
Status: Available
Owner:
johnidel@chromium.org
Cc:
█ csharrison@google.com
ryansturm@chromium.org
skare@chromium.org
isherman@chromium.org
asvitk...@chromium.org
█ rkaplow@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Migrate PageLoadMetricsObservers measuring network/page bytes to OnResourceDataUseObserved

Project Member Reported by johnidel@chromium.org, Oct 10 
Currently a large number of PageLoadMetricsObservers are recording network/page bytes histograms using |raw_body_bytes| from PageLoadMetricsObserver::OnLoadedResource. This is missing all bytes for resource requests that canceled or were ongoing when metrics are recorded. Notably videos played via src are treated as one ongoing/incomplete request until the entire video file is buffered (unless it is paused), and are unaccounted for when using this pattern.

Consider migrating these observers to use OnResourceDataUseObserved which is populated via transfer size updates and captures bytes used for incomplete requests. 

Observers currently using OnLoadedResource pattern to measure bytes:
- CorePageLoadMetricsObserver
- LofiPageLoadMetricsObserver
- MediaPageLoadMetricsObserver
- PageCappingPageLoadMetricsObserver
- TabRestorePageLoadMetricsObserver
- UkmPageLoadMetricsObserver

Comment 1 by ryansturm@chromium.org, Oct 10

Status: Available (was: Untriaged)
I'm going to fix the PageCapping observer for this 71, but I can do the others in 72 if nobody else wants to own this.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b555a896c90aa549c149a5c423682f3edfa6a11f

commit b555a896c90aa549c149a5c423682f3edfa6a11f
Author: Ryan Sturm <ryansturm@chromium.org>
Date: Wed Oct 10 23:34:45 2018

Moving Page Capping data use detection off resource finished

This adds continuous data use updates instead of only listening to
resource completion events. This is helpful for media requests
especially.

Bug: 894168
Change-Id: Ide9c02b30396217aebef94f1ff681f99625b2d56
Reviewed-on: https://chromium-review.googlesource.com/c/1274385
Commit-Queue: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598571}
[modify] https://crrev.com/b555a896c90aa549c149a5c423682f3edfa6a11f/chrome/browser/page_load_metrics/observers/page_capping_page_load_metrics_observer.cc
[modify] https://crrev.com/b555a896c90aa549c149a5c423682f3edfa6a11f/chrome/browser/page_load_metrics/observers/page_capping_page_load_metrics_observer.h
[modify] https://crrev.com/b555a896c90aa549c149a5c423682f3edfa6a11f/chrome/browser/page_load_metrics/observers/page_capping_page_load_metrics_observer_unittest.cc

Comment 3 by johnidel@chromium.org, Oct 26

Owner: johnidel@chromium.org
I'll own the remaining changes.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69c212903301081d93e4132fbf968ffb9b90e3e0

commit 69c212903301081d93e4132fbf968ffb9b90e3e0
Author: John Delaney <johnidel@chromium.org>
Date: Thu Nov 29 19:45:54 2018

Add browsertest for PLM ResourceDataUpdates with chunked response

Add a browsertest to verify that we are not counting overhead
for responses with "Transfer-Encoding: chunked" towards network body
bytes. Rename PageLoadMetricsTestWaiter methods to use "network bytes"
instead of "resource bytes".

Bug: 894168
Change-Id: Id83af484f210063761dcbf4552406f08cdd5a171
Reviewed-on: https://chromium-review.googlesource.com/c/1351383
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: John Delaney <johnidel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612322}
[modify] https://crrev.com/69c212903301081d93e4132fbf968ffb9b90e3e0/chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
[modify] https://crrev.com/69c212903301081d93e4132fbf968ffb9b90e3e0/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
[modify] https://crrev.com/69c212903301081d93e4132fbf968ffb9b90e3e0/chrome/browser/page_load_metrics/page_load_metrics_test_waiter.cc
[modify] https://crrev.com/69c212903301081d93e4132fbf968ffb9b90e3e0/chrome/browser/page_load_metrics/page_load_metrics_test_waiter.h
[modify] https://crrev.com/69c212903301081d93e4132fbf968ffb9b90e3e0/chrome/common/page_load_metrics/page_load_metrics.mojom
[modify] https://crrev.com/69c212903301081d93e4132fbf968ffb9b90e3e0/chrome/renderer/page_load_metrics/page_resource_data_use.cc
[modify] https://crrev.com/69c212903301081d93e4132fbf968ffb9b90e3e0/chrome/renderer/page_load_metrics/page_resource_data_use.h
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 8 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af544c05560d6b36a5936450d16daf39c7e6f7af

commit af544c05560d6b36a5936450d16daf39c7e6f7af
Author: rajendrant <rajendrant@chromium.org>
Date: Sat Dec 08 03:33:45 2018

Record network bytes data use using OnResourceDataUpdate

Record bytes reported by OnResourceDataUpdate observer callback which is received
pre and post mojo. This is helpful for media requests which do not complete. This
is also needed post-mojo since OnLoadedResource will not be called.

TBR=mpearson@chromium.org

Bug:  912737 , 894168
Change-Id: I290d5d2438f0838daafb92e2b264e15ef9af7e16
Reviewed-on: https://chromium-review.googlesource.com/c/1368593
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614936}
[modify] https://crrev.com/af544c05560d6b36a5936450d16daf39c7e6f7af/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
[modify] https://crrev.com/af544c05560d6b36a5936450d16daf39c7e6f7af/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
[modify] https://crrev.com/af544c05560d6b36a5936450d16daf39c7e6f7af/tools/metrics/histograms/histograms.xml
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 11 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c22d02669f1bcffd045fee5e6876760748ffb96c

commit c22d02669f1bcffd045fee5e6876760748ffb96c
Author: John Delaney <johnidel@chromium.org>
Date: Tue Dec 11 00:49:02 2018

Migrate PLM observers counting bytes to OnResourceDataUpdate

Move observers recording these byte histograms to OnResourceDataUpdate
instead of OnLoadedResource. OnResourceDataUpdate contains information
on incomplete resources, and thus should be the interface for counting
bytes moving forward.

Bug: 894168
Change-Id: Ida8f79271af918a2778b0e06005884d3cf672222
Reviewed-on: https://chromium-review.googlesource.com/c/1302660
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: rajendrant <rajendrant@chromium.org>
Commit-Queue: John Delaney <johnidel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615352}
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.cc
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer.h
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.h
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.cc
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer.h
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/common/page_load_metrics/test/page_load_metrics_test_util.cc
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/chrome/common/page_load_metrics/test/page_load_metrics_test_util.h
[modify] https://crrev.com/c22d02669f1bcffd045fee5e6876760748ffb96c/tools/metrics/histograms/histograms.xml
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 12

Labels: merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f239aa492cf46f8bea9be1e9af3b924ee10f332

commit 8f239aa492cf46f8bea9be1e9af3b924ee10f332
Author: rajendrant <rajendrant@chromium.org>
Date: Wed Dec 12 19:09:02 2018

Record network bytes data use using OnResourceDataUpdate

Record bytes reported by OnResourceDataUpdate observer callback which is received
pre and post mojo. This is helpful for media requests which do not complete. This
is also needed post-mojo since OnLoadedResource will not be called.

TBR=mpearson@chromium.org

Bug:  912737 , 894168
Change-Id: I290d5d2438f0838daafb92e2b264e15ef9af7e16
Reviewed-on: https://chromium-review.googlesource.com/c/1368593
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#614936}(cherry picked from commit af544c05560d6b36a5936450d16daf39c7e6f7af)
Reviewed-on: https://chromium-review.googlesource.com/c/1374408
Reviewed-by: rajendrant <rajendrant@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#296}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/8f239aa492cf46f8bea9be1e9af3b924ee10f332/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
[modify] https://crrev.com/8f239aa492cf46f8bea9be1e9af3b924ee10f332/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
[modify] https://crrev.com/8f239aa492cf46f8bea9be1e9af3b924ee10f332/tools/metrics/histograms/histograms.xml
Project Member

Comment 8 by cr-audit...@appspot.gserviceaccount.com, Dec 19

Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/8f239aa492cf46f8bea9be1e9af3b924ee10f332

Commit: 8f239aa492cf46f8bea9be1e9af3b924ee10f332
Author: rajendrant@chromium.org
Commiter: rajendrant@chromium.org
Date: 2018-12-12 19:09:02 +0000 UTC

Record network bytes data use using OnResourceDataUpdate

Record bytes reported by OnResourceDataUpdate observer callback which is received
pre and post mojo. This is helpful for media requests which do not complete. This
is also needed post-mojo since OnLoadedResource will not be called.

TBR=mpearson@chromium.org

Bug:  912737 , 894168
Change-Id: I290d5d2438f0838daafb92e2b264e15ef9af7e16
Reviewed-on: https://chromium-review.googlesource.com/c/1368593
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#614936}(cherry picked from commit af544c05560d6b36a5936450d16daf39c7e6f7af)
Reviewed-on: https://chromium-review.googlesource.com/c/1374408
Reviewed-by: rajendrant <rajendrant@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#296}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
►

Sign in to add a comment