Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2453653002:
Separate out observation sources as either HTTP layer or transport layer (Closed)
DescriptionSeparate out observation sources as either HTTP layer or transport layer
Separate out observation sources as either HTTP layer or
transport layer. Majority of the current observation
sources (e.g., cached estimate, external estimate) are not
broken down by which networking layer they were computed
at.
This CL breaks down the observation sources by the layer
(either HTTP or transport layer). e.g., this will be useful
for observers to distinguish between a cached HTTP RTT
estimate vs. a cached transport RTT estimate.
when NQE caches both HTTP RTT, and transport RTT.
As a side-effect, this CL will prevent the observers from
distinguishing between the RTT from a TCP source vs. a QUIC
source since both of them will now be bundled into
NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
BUG=490870
Committed: https://crrev.com/c1e6441f00d0f422f57235ef1657a9317ea2d7df
Cr-Commit-Position: refs/heads/master@{#430297}
Patch Set 1 #
Total comments: 2
Patch Set 2 : ps #
Total comments: 4
Patch Set 3 : Addressed bengr comments #
Messages
Total messages: 38 (26 generated)
Description was changed from ========== Separate out observation sources BUG= ========== to ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. BUG= ==========
Description was changed from ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. BUG= ========== to ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. As a side-effect, this CL will prevent the observers from distinguishing between the RTT from a TCP source vs. a QUIC source since both of them will now be bundled into NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT. BUG= ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by tbansal@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...
Patchset #1 (id:20001) has been deleted
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. thanks.
Description was changed from ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. As a side-effect, this CL will prevent the observers from distinguishing between the RTT from a TCP source vs. a QUIC source since both of them will now be bundled into NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT. BUG= ========== to ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. As a side-effect, this CL will prevent the observers from distinguishing between the RTT from a TCP source vs. a QUIC source since both of them will now be bundled into NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT. BUG=490870 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
If embedders are aware of the change and it won't break them lgtm % nit https://codereview.chromium.org/2453653002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_observation_source.h (right): https://codereview.chromium.org/2453653002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_observation_source.h:22: // DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_TCP, nit:s/DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_TCP,/DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_TCP = 1, and the rest of the deprecated entries
On 2016/10/31 21:13:46, Ryan Sturm wrote: > If embedders are aware of the change and it won't break them lgtm % nit Yup, I already talked with embedders. There are no objections. I am going to update their code before landing this CL.
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. thanks. https://codereview.chromium.org/2453653002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_observation_source.h (right): https://codereview.chromium.org/2453653002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_observation_source.h:22: // DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_TCP, On 2016/10/31 21:13:46, Ryan Sturm wrote: > nit:s/DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_TCP,/DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_TCP > = 1, > > and the rest of the deprecated entries Done.
https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1119: NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT); I don't understand why this is necessary. Won't some consumers care about QUIC vs TCP? What prompted this change? https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_observation_source.h (right): https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_observation_source.h:22: // DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_TCP = 1, Why not just rename this as TRANSPORT? In general, why all the deprecation?
https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1119: NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT); On 2016/11/04 16:38:13, bengr wrote: > I don't understand why this is necessary. Won't some consumers care about QUIC > vs TCP? My plan is to cache the transport RTT and HTTP RTT estimate. If we separate out the TCP and QUIC, then we will need to cache the 3 separately. I do not know of any consumers that are interested in separating these out. > What prompted this change? Without this CL, it is not possible to cache the transport RTT and HTTP RTT estimate separately since currently there is only one enum available for cached RTT. https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_observation_source.h (right): https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... net/nqe/network_quality_observation_source.h:22: // DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_TCP = 1, On 2016/11/04 16:38:13, bengr wrote: > Why not just rename this as TRANSPORT? I can do this. I just wanted to avoid confusion of using the same enum value for 2 different things (TCP vs. TCP+QUIC). > In general, why all the deprecation? I would still need to deprecate other enums because they (e.g., NETWORK_QUALITY_OBSERVATION_SOURCE_CACHED_ESTIMATE, NETWORK_QUALITY_OBSERVATION_SOURCE_DEFAULT_FROM_PLATFORM) have been split into 2 (one for HTTP, one for transport).
On 2016/11/04 20:11:20, tbansal1 wrote: > https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... > File net/nqe/network_quality_estimator.cc (right): > > https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... > net/nqe/network_quality_estimator.cc:1119: > NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT); > On 2016/11/04 16:38:13, bengr wrote: > > I don't understand why this is necessary. Won't some consumers care about QUIC > > vs TCP? > My plan is to cache the transport RTT and HTTP RTT estimate. If we separate out > the TCP and QUIC, then we will need to cache the 3 separately. I do not know of > any consumers that are interested in separating these out. > > What prompted this change? > Without this CL, it is not possible to cache the transport RTT and HTTP RTT > estimate separately since currently there is only one enum available for cached > RTT. > > https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... > File net/nqe/network_quality_observation_source.h (right): > > https://codereview.chromium.org/2453653002/diff/60001/net/nqe/network_quality... > net/nqe/network_quality_observation_source.h:22: // > DEPRECATED_NETWORK_QUALITY_OBSERVATION_SOURCE_TCP = 1, > On 2016/11/04 16:38:13, bengr wrote: > > Why not just rename this as TRANSPORT? > I can do this. I just wanted to avoid confusion of using the same enum value for > 2 different things (TCP vs. TCP+QUIC). > > In general, why all the deprecation? > I would still need to deprecate other enums because they (e.g., > NETWORK_QUALITY_OBSERVATION_SOURCE_CACHED_ESTIMATE, > NETWORK_QUALITY_OBSERVATION_SOURCE_DEFAULT_FROM_PLATFORM) have been split into > 2 (one for HTTP, one for transport). Do whatever is best for you.
lgtm
The CQ bit was checked by tbansal@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 ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. As a side-effect, this CL will prevent the observers from distinguishing between the RTT from a TCP source vs. a QUIC source since both of them will now be bundled into NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT. BUG=490870 ========== to ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. As a side-effect, this CL will prevent the observers from distinguishing between the RTT from a TCP source vs. a QUIC source since both of them will now be bundled into NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ==========
The CQ bit was checked by tbansal@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...
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #3 (id:100001) has been deleted
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: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2453653002/#ps120001 (title: "Addressed bengr comments")
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 ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. As a side-effect, this CL will prevent the observers from distinguishing between the RTT from a TCP source vs. a QUIC source since both of them will now be bundled into NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ========== to ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. As a side-effect, this CL will prevent the observers from distinguishing between the RTT from a TCP source vs. a QUIC source since both of them will now be bundled into NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. As a side-effect, this CL will prevent the observers from distinguishing between the RTT from a TCP source vs. a QUIC source since both of them will now be bundled into NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 ========== to ========== Separate out observation sources as either HTTP layer or transport layer Separate out observation sources as either HTTP layer or transport layer. Majority of the current observation sources (e.g., cached estimate, external estimate) are not broken down by which networking layer they were computed at. This CL breaks down the observation sources by the layer (either HTTP or transport layer). e.g., this will be useful for observers to distinguish between a cached HTTP RTT estimate vs. a cached transport RTT estimate. when NQE caches both HTTP RTT, and transport RTT. As a side-effect, this CL will prevent the observers from distinguishing between the RTT from a TCP source vs. a QUIC source since both of them will now be bundled into NETWORK_QUALITY_OBSERVATION_SOURCE_TRANSPORT. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=490870 Committed: https://crrev.com/c1e6441f00d0f422f57235ef1657a9317ea2d7df Cr-Commit-Position: refs/heads/master@{#430297} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c1e6441f00d0f422f57235ef1657a9317ea2d7df Cr-Commit-Position: refs/heads/master@{#430297} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
