Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a congestionWindowInBytes method to Envoy::Network::Connection #20105

Merged
merged 8 commits into from
Mar 1, 2022

Conversation

wu-bin
Copy link
Contributor

@wu-bin wu-bin commented Feb 24, 2022

Add a congestionWindowInBytes() method to Envoy::Network::Connection.

  • QUIC connections get the congestion window from QuicSentPacketManager::GetCongestionWindowInBytes().
  • TCP connections get the congestion window from tcp_info.tcpi_snd_cwnd.

Also Implement Envoy::Network::Connection::lastRoundTripTime() for QUIC connections.

Commit Message: Add a congestionWindowInBytes method to Envoy::Network::Connection
Risk Level: Low
Testing: Unit test
Platform Specific Features: Getting congestion window for TCP connections may not be supported by all platforms.

- QUIC connections get the congestion window from
  QuicSentPacketManager::GetCongestionWindowInBytes().
- TCP connections get the congestion window from tcp_info.tcpi_snd_cwnd.

Signed-off-by: Bin Wu <wub@google.com>
connections.

Signed-off-by: Bin Wu <wub@google.com>
@wu-bin wu-bin requested a review from rojkov as a code owner February 24, 2022 17:21
Signed-off-by: Bin Wu <wub@google.com>
@wu-bin
Copy link
Contributor Author

wu-bin commented Feb 24, 2022

/assign @danzh2010

@wu-bin
Copy link
Contributor Author

wu-bin commented Feb 24, 2022

/assign @mattklein123

@@ -788,6 +788,10 @@ void ConnectionImpl::configureInitialCongestionWindow(uint64_t bandwidth_bits_pe
return transport_socket_->configureInitialCongestionWindow(bandwidth_bits_per_sec, rtt);
}

absl::optional<uint64_t> ConnectionImpl::congestionWindowInBytes() const {
return socket_->congestionWindowInBytes();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually can directly access socket->ioHandle() here. Can we skip ConnectionSocket::congestionWindowInBytes() interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm following ConnectionImpl::lastRoundTripTime() here. I don't mind directly access socket->ioHandle() from here, but it's better for lastRoundTripTime and congestionWindowInBytes to do things consistently.

@mattklein123 : WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No real opinion. Whatever you both think is fine with me though I agree about being consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Matt. I talked with Dan offline and we'll leave it as is.

test/common/network/connection_impl_test.cc Show resolved Hide resolved
@@ -19,6 +19,9 @@ namespace Api {

struct EnvoyTcpInfo {
std::chrono::microseconds tcpi_rtt;
// Congestion window, in bytes. Note that posix's TCP_INFO socket option returns cwnd in packets,
// we multiply it by 1460(the default TCP MSS) to get bytes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1460 default MSS seems fine for internet traffic. I'm wondering if it worth to make it configurable for user controlled network or ENVOY_BUG if TCP MSS is not 1460. @mattklein123 for thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, it seems tcp_info.tcpi_snd_mss contains the MSS value, so I changed to it instead of 1460.

instead of the fixed value 1460.

Signed-off-by: Bin Wu <wub@google.com>
Signed-off-by: Bin Wu <wub@google.com>
Signed-off-by: Bin Wu <wub@google.com>
Copy link
Contributor

@danzh2010 danzh2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mattklein123 mattklein123 merged commit 563d36b into envoyproxy:main Mar 1, 2022
rexnp pushed a commit to rexnp/envoy that referenced this pull request Mar 2, 2022
…nvoyproxy#20105)

Signed-off-by: Bin Wu <wub@google.com>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>
wrowe pushed a commit that referenced this pull request Mar 15, 2022
* test: adding a multi-envoy test (#20016)

Functionally this

handles the multi-envoy signal handler crash
skips instantiating a runtime singleton (off by default, must stay off until remove global runtime: rebase runtime features on ABSL_flags #19847 is closed)
Multi-envoy does not correctly support runtime flags or deprecation stats due to #19847 being incomplete. It can still handle proxy traffic client - L1 - L2 - upstream as shown in test.

Risk Level: low
Testing: yes
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#2003

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* Add a congestionWindowInBytes method to Envoy::Network::Connection (#20105)

Signed-off-by: Bin Wu <wub@google.com>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* Update QUICHE from 50f15e7a5 to cf1588207 (#20154)

https://github.com/google/quiche/compare/50f15e7a5..cf1588207

$ git log 50f15e7a5..cf1588207 --date=short --no-merges --format="%ad %al %s"

2022-02-28 wub Deprecate --gfe2_reloadable_flag_quic_crypto_noop_if_disconnected_after_process_chlo.
2022-02-27 vasilvv Remove QuicheMemSlice(QuicUniqueBufferPtr, size_t) constructor.
2022-02-26 fayang Use std::string instead of absl::string_view in CryptoBufferMap.
2022-02-25 bnc Ignore incoming HTTP/3 MAX_PUSH_ID frames.
2022-02-25 bnc Remove Http3DebugVisitor::OnMaxPushIdFrameSent().
2022-02-25 bnc Remove QuicSpdySession::CanCreatePushStreamWithId().
2022-02-25 fayang Deprecate gfe2_reloadable_flag_quic_single_ack_in_packet2.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* build(deps): bump actions/stale from 4.1.0 to 5 (#20159)

Bumps [actions/stale](https://github.com/actions/stale) from 4.1.0 to 5.
- [Release notes](https://github.com/actions/stale/releases)
- [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md)
- [Commits](actions/stale@v4.1.0...v5)

---
updated-dependencies:
- dependency-name: actions/stale
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* admin: improve test coverage and increase the coverage-percent threshold (#20025)

Adds a missing test for recent lookups now that there are no more fake symbol tables. Adds tests for a variety of override methods defined in admin.h that were previously hard to hit.

Adds a benchmark test to establish a baseline for the speedups in #19693

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* test: removing a bunch of direct runtime singleton access (#19993)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* build(deps): bump grpcio-tools in /examples/grpc-bridge/client (#20040)

Bumps [grpcio-tools](https://github.com/grpc/grpc) from 1.43.0 to 1.44.0.
- [Release notes](https://github.com/grpc/grpc/releases)
- [Changelog](https://github.com/grpc/grpc/blob/master/doc/grpc_release_schedule.md)
- [Commits](grpc/grpc@v1.43.0...v1.44.0)

---
updated-dependencies:
- dependency-name: grpcio-tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds to spellcheck

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* xray tracer: set subsegment type for child spans (#2)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* Xray subsegment (#3)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* Xray subsegment (#4)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds to spell check dictionary

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* fixes spellcheck

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds to spellcheck

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

xray tracer: set subsegment type for child spans (#2)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

Xray subsegment (#3)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

Xray subsegment (#4)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds to spell check dictionary

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

fixes spellcheck

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* fixes spell check

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

Co-authored-by: alyssawilk <alyssar@chromium.org>
Co-authored-by: Bin Wu <46450037+wu-bin@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Joshua Marantz <jmarantz@google.com>
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
…yproxy#20170)

* test: adding a multi-envoy test (envoyproxy#20016)

Functionally this

handles the multi-envoy signal handler crash
skips instantiating a runtime singleton (off by default, must stay off until remove global runtime: rebase runtime features on ABSL_flags envoyproxy#19847 is closed)
Multi-envoy does not correctly support runtime flags or deprecation stats due to envoyproxy#19847 being incomplete. It can still handle proxy traffic client - L1 - L2 - upstream as shown in test.

Risk Level: low
Testing: yes
Docs Changes: n/a
Release Notes: n/a
Part of envoyproxy/envoy-mobile#2003

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* Add a congestionWindowInBytes method to Envoy::Network::Connection (envoyproxy#20105)

Signed-off-by: Bin Wu <wub@google.com>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* Update QUICHE from 50f15e7a5 to cf1588207 (envoyproxy#20154)

https://github.com/google/quiche/compare/50f15e7a5..cf1588207

$ git log 50f15e7a5..cf1588207 --date=short --no-merges --format="%ad %al %s"

2022-02-28 wub Deprecate --gfe2_reloadable_flag_quic_crypto_noop_if_disconnected_after_process_chlo.
2022-02-27 vasilvv Remove QuicheMemSlice(QuicUniqueBufferPtr, size_t) constructor.
2022-02-26 fayang Use std::string instead of absl::string_view in CryptoBufferMap.
2022-02-25 bnc Ignore incoming HTTP/3 MAX_PUSH_ID frames.
2022-02-25 bnc Remove Http3DebugVisitor::OnMaxPushIdFrameSent().
2022-02-25 bnc Remove QuicSpdySession::CanCreatePushStreamWithId().
2022-02-25 fayang Deprecate gfe2_reloadable_flag_quic_single_ack_in_packet2.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* build(deps): bump actions/stale from 4.1.0 to 5 (envoyproxy#20159)

Bumps [actions/stale](https://github.com/actions/stale) from 4.1.0 to 5.
- [Release notes](https://github.com/actions/stale/releases)
- [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md)
- [Commits](actions/stale@v4.1.0...v5)

---
updated-dependencies:
- dependency-name: actions/stale
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* admin: improve test coverage and increase the coverage-percent threshold (envoyproxy#20025)

Adds a missing test for recent lookups now that there are no more fake symbol tables. Adds tests for a variety of override methods defined in admin.h that were previously hard to hit.

Adds a benchmark test to establish a baseline for the speedups in envoyproxy#19693

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* test: removing a bunch of direct runtime singleton access (envoyproxy#19993)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* build(deps): bump grpcio-tools in /examples/grpc-bridge/client (envoyproxy#20040)

Bumps [grpcio-tools](https://github.com/grpc/grpc) from 1.43.0 to 1.44.0.
- [Release notes](https://github.com/grpc/grpc/releases)
- [Changelog](https://github.com/grpc/grpc/blob/master/doc/grpc_release_schedule.md)
- [Commits](grpc/grpc@v1.43.0...v1.44.0)

---
updated-dependencies:
- dependency-name: grpcio-tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds to spellcheck

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* xray tracer: set subsegment type for child spans (#2)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* Xray subsegment (#3)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* Xray subsegment (#4)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds to spell check dictionary

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* fixes spellcheck

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds to spellcheck

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

xray tracer: set subsegment type for child spans (#2)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

Xray subsegment (#3)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

Xray subsegment (#4)

* xray tracer: set subsegment type for child spans

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds test coverage

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates xray subsegment name to use operation name (instead of parent's span name)

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* updates doc

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* adds to spell check dictionary

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

fixes spellcheck

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

* fixes spell check

Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

Co-authored-by: alyssawilk <alyssar@chromium.org>
Co-authored-by: Bin Wu <46450037+wu-bin@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Joshua Marantz <jmarantz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants