Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466343003:
[Offline Pages] Adds UMA for effective network connection on api calls. (Closed)
|
Created:
4 years, 1 month ago by dougarnett Modified:
4 years, 1 month ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, asvitkine+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Adds UMA for effective network connection on api calls.
Captures the NetworkQualityEstimator connection type on SavePageLater,
RemoveRequests, PauseRequest, and ResumeRequests to shed some insight
on the conditions under which the user is triggering these apis.
BUG=655341
Committed: https://crrev.com/75e325cb30270ec359ee79055e825f0e3cfb334a
Cr-Commit-Position: refs/heads/master@{#429283}
Patch Set 1 #Patch Set 2 : Format update #
Total comments: 2
Patch Set 3 : Removed a couple TODO comments and a log message #
Messages
Total messages: 23 (11 generated)
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + dewittj@chromium.org, petewil@chromium.org
Btw, have open question (in TODO) about whether we want to take some effort to record RemoveRequests network quality per namespace or not bother. API only takes request ids so namespace not readily available.
On 2016/11/01 17:51:40, dougarnett wrote: > Btw, have open question (in TODO) about whether we want to > take some effort to record RemoveRequests network quality > per namespace or not bother. API only takes request ids so > namespace not readily available. I'm not sure whether I am worried about recording network quality when you remove requests. I'm mostly concerned about quality when SavePageLater is called.
https://codereview.chromium.org/2466343003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2466343003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:353: if (network_quality_estimator_) { These three added sections are very similar - can you refactor them to use a common function?
this lgtm, can you point me to a doc or explain what we are trying to measure exactly here? https://codereview.chromium.org/2466343003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2466343003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:353: if (network_quality_estimator_) { On 2016/11/01 19:58:47, Pete Williamson wrote: > These three added sections are very similar - can you refactor them to use a > common function? I suspect there won't be much to gain from that, since you can no longer use the UMA macros.. Though an interesting one would be to add a histogram value for "no quality estimator"
lgtm
On 2016/11/01 21:52:41, dewittj wrote: > this lgtm, can you point me to a doc or explain what we are trying to measure > exactly here? Yeah, fair question. Generally, it would be great to get some idea of what caliber of networks our users actually experience when we get SavePageLater requests (after tab didn't succeed for downloads; hopefully good connection for agsa results). This will help us tune timeouts, retries, RMD. Second, anytime a user does a Pause/Resume/Cancel, they may be expressing some impatience/frustration. Do we actually have a good connection (an NQE judged effective one, not just a connection) but we are waiting on GcmNetworkManger to trigger? One thing we want to motivate is whether adding an observer on the NetworkQualityEstimator may be worthwhile to trigger background loads sooner upon connecting.
dougarnett@chromium.org changed reviewers: + holte@chromium.org
FYI Pete, I removed the TODO and the quality log message now that this UMA.
The CQ bit was checked by dougarnett@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...
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 dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2466343003/#ps40001 (title: "Removed a couple TODO comments and a log message")
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 ========== [Offline Pages] Adds UMA for effective network connection on api calls. Captures the NetworkQualityEstimator connection type on SavePageLater, RemoveRequests, PauseRequest, and ResumeRequests to shed some insight on the conditions under which the user is triggering these apis. BUG=655341 ========== to ========== [Offline Pages] Adds UMA for effective network connection on api calls. Captures the NetworkQualityEstimator connection type on SavePageLater, RemoveRequests, PauseRequest, and ResumeRequests to shed some insight on the conditions under which the user is triggering these apis. BUG=655341 Committed: https://crrev.com/75e325cb30270ec359ee79055e825f0e3cfb334a Cr-Commit-Position: refs/heads/master@{#429283} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/75e325cb30270ec359ee79055e825f0e3cfb334a Cr-Commit-Position: refs/heads/master@{#429283} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
