Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737483004:
Reporting: Implement endpoint manager. (Closed)
|
Created:
3 years, 9 months ago by Julia Tuttle Modified:
3 years, 8 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReporting: Implement endpoint manager.
Reporting is a spec for delivering out-of-band reports from various
other parts of the browser. See http://wicg.github.io/reporting/ for
the spec, or https://goo.gl/pygX5I for details of the planned
implementation in Chromium.
The endpoint manager keeps track of which endpoints have active
delivery attempts in progress, and chooses endpoints from those
configured in the cache, filtering out busy, failed, or expired
ones.
BUG=704259
Review-Url: https://codereview.chromium.org/2737483004
Cr-Commit-Position: refs/heads/master@{#461449}
Committed: https://chromium.googlesource.com/chromium/src/+/add0a38b6ba104546419dc750650a3b09fa81810
Patch Set 1 #Patch Set 2 : format #Patch Set 3 : rebase #Patch Set 4 : DISALLOW_COPY_AND_ASSIGN #Patch Set 5 : rebase #Patch Set 6 : rebase #
Total comments: 4
Patch Set 7 : Make requested changes. #Patch Set 8 : format, rebase #
Total comments: 6
Patch Set 9 : Make requested changes. #Patch Set 10 : rebase #
Dependent Patchsets: Messages
Total messages: 41 (30 generated)
Description was changed from ========== Reporting: Create EndpointManager. ========== to ========== Reporting: Create EndpointManager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The Uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. The EndpointManager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG= ==========
juliatuttle@chromium.org changed reviewers: + shivanisha@chromium.org
PTAL, shivanisha.
Description was changed from ========== Reporting: Create EndpointManager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The Uploader handles the network side of report delivery: it takes the assembled JSON and an endpoint URL, and handles sending the URL request and categorizing the response according to the Reporting spec. The EndpointManager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG= ========== to ========== Reporting: Create EndpointManager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The EndpointManager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG= ==========
The CQ bit was checked by juliatuttle@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by juliatuttle@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: This issue passed the CQ dry run.
jkarlin@chromium.org changed reviewers: + jkarlin@chromium.org
Description was changed from ========== Reporting: Create EndpointManager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The EndpointManager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG= ========== to ========== Reporting: Create endpoint manager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The EndpointManager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG= ==========
The CQ bit was checked by juliatuttle@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: This issue passed the CQ dry run.
Looking good. Left some comments. https://codereview.chromium.org/2737483004/diff/100001/net/reporting/reportin... File net/reporting/reporting_endpoint_manager.h (right): https://codereview.chromium.org/2737483004/diff/100001/net/reporting/reportin... net/reporting/reporting_endpoint_manager.h:31: const BackoffEntry::Policy* backoff_policy); Document the lifetime of clock, cache and backoff_policy since their raw pointers are being passed here. https://codereview.chromium.org/2737483004/diff/100001/net/reporting/reportin... net/reporting/reporting_endpoint_manager.h:64: std::map<GURL, std::unique_ptr<net::BackoffEntry>> endpoint_backoff_; if there is no ordering needed, can we use unordered_set and unordered_map respectively for the above two fields.
The CQ bit was checked by juliatuttle@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2737483004/diff/100001/net/reporting/reportin... File net/reporting/reporting_endpoint_manager.h (right): https://codereview.chromium.org/2737483004/diff/100001/net/reporting/reportin... net/reporting/reporting_endpoint_manager.h:31: const BackoffEntry::Policy* backoff_policy); On 2017/03/20 19:06:51, shivanisha wrote: > Document the lifetime of clock, cache and backoff_policy since their raw > pointers are being passed here. Done. https://codereview.chromium.org/2737483004/diff/100001/net/reporting/reportin... net/reporting/reporting_endpoint_manager.h:64: std::map<GURL, std::unique_ptr<net::BackoffEntry>> endpoint_backoff_; On 2017/03/20 19:06:51, shivanisha wrote: > if there is no ordering needed, can we use unordered_set and unordered_map > respectively for the above two fields. Can't; GURL doesn't have a hash.
Description was changed from ========== Reporting: Create endpoint manager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/. The EndpointManager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG= ========== to ========== Reporting: Implement endpoint manager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. The endpoint manager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG= ==========
Description was changed from ========== Reporting: Implement endpoint manager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. The endpoint manager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG= ========== to ========== Reporting: Implement endpoint manager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. The endpoint manager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG=704259 ==========
On 2017/03/21 at 21:43:03, juliatuttle wrote: > https://codereview.chromium.org/2737483004/diff/100001/net/reporting/reportin... > File net/reporting/reporting_endpoint_manager.h (right): > > https://codereview.chromium.org/2737483004/diff/100001/net/reporting/reportin... > net/reporting/reporting_endpoint_manager.h:31: const BackoffEntry::Policy* backoff_policy); > On 2017/03/20 19:06:51, shivanisha wrote: > > Document the lifetime of clock, cache and backoff_policy since their raw > > pointers are being passed here. > > Done. > > https://codereview.chromium.org/2737483004/diff/100001/net/reporting/reportin... > net/reporting/reporting_endpoint_manager.h:64: std::map<GURL, std::unique_ptr<net::BackoffEntry>> endpoint_backoff_; > On 2017/03/20 19:06:51, shivanisha wrote: > > if there is no ordering needed, can we use unordered_set and unordered_map > > respectively for the above two fields. > > Can't; GURL doesn't have a hash. lgtm
juliatuttle@chromium.org changed reviewers: + rdsmith@chromium.org
PTAL, jkarlin or rdsmith (for committer review).
On 2017/03/30 18:08:27, Julia Tuttle wrote: > PTAL, jkarlin or rdsmith (for committer review). Ping! This is now next on my queue to commit.
LGTM % not referencing BackoffEntry in the consumer method doc--I'm happy going with your judgement for the other nits. https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... File net/reporting/reporting_endpoint_manager.h (right): https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... net/reporting/reporting_endpoint_manager.h:62: // Informs the BackoffEntry for |endpoint| of a successful or not (according BackoffEntry isn't an exported concept, so shouldn't be referenced in consumer documentation. "Inform the manager of a request result (successful or unsuccessful) for use in managing exponential backoff" would be better. https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... net/reporting/reporting_endpoint_manager.h:63: // to |succeeded|) request. nit, suggestion: I"d phrase this as "of a successful or unsuccessful request." I think the use of the bool is pretty obvious in context and doesn't need to be referenced explicitly. https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... File net/reporting/reporting_endpoint_manager_unittest.cc (right): https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... net/reporting/reporting_endpoint_manager_unittest.cc:104: manager_.InformOfEndpointRequest(kEndpoint_, false); This seems like the only call to InformOfEnpointRequest in this file. Worthwhile having a test to make sure that a successful request has the appropriate effect on the backoff?
Thanks, rdsmith; take a look at the changes if you like. https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... File net/reporting/reporting_endpoint_manager.h (right): https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... net/reporting/reporting_endpoint_manager.h:62: // Informs the BackoffEntry for |endpoint| of a successful or not (according On 2017/03/31 15:14:08, Randy Smith (Not in Mondays) wrote: > BackoffEntry isn't an exported concept, so shouldn't be referenced in consumer > documentation. "Inform the manager of a request result (successful or > unsuccessful) for use in managing exponential backoff" would be better. Done. https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... net/reporting/reporting_endpoint_manager.h:63: // to |succeeded|) request. On 2017/03/31 15:14:08, Randy Smith (Not in Mondays) wrote: > nit, suggestion: I"d phrase this as "of a successful or unsuccessful request." > I think the use of the bool is pretty obvious in context and doesn't need to be > referenced explicitly. Done. https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... File net/reporting/reporting_endpoint_manager_unittest.cc (right): https://codereview.chromium.org/2737483004/diff/140001/net/reporting/reportin... net/reporting/reporting_endpoint_manager_unittest.cc:104: manager_.InformOfEndpointRequest(kEndpoint_, false); On 2017/03/31 15:14:09, Randy Smith (Not in Mondays) wrote: > This seems like the only call to InformOfEnpointRequest in this file. > Worthwhile having a test to make sure that a successful request has the > appropriate effect on the backoff? Done, grumpily. This now tests fail, fail, success, success, fail, and makes sure the failures are followed by 1x, 2x, and 1x backoffs respectively, which should ensure the success calls are working too.
The CQ bit was checked by juliatuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shivanisha@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2737483004/#ps160001 (title: "Make requested changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by juliatuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shivanisha@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2737483004/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491230780848380,
"parent_rev": "f37ff83e96bc94df6f3320c15ae3c3c1a3e302fb", "commit_rev":
"add0a38b6ba104546419dc750650a3b09fa81810"}
Message was sent while issue was closed.
Description was changed from ========== Reporting: Implement endpoint manager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. The endpoint manager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG=704259 ========== to ========== Reporting: Implement endpoint manager. Reporting is a spec for delivering out-of-band reports from various other parts of the browser. See http://wicg.github.io/reporting/ for the spec, or https://goo.gl/pygX5I for details of the planned implementation in Chromium. The endpoint manager keeps track of which endpoints have active delivery attempts in progress, and chooses endpoints from those configured in the cache, filtering out busy, failed, or expired ones. BUG=704259 Review-Url: https://codereview.chromium.org/2737483004 Cr-Commit-Position: refs/heads/master@{#461449} Committed: https://chromium.googlesource.com/chromium/src/+/add0a38b6ba104546419dc750650... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/add0a38b6ba104546419dc750650... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
