-
Notifications
You must be signed in to change notification settings - Fork 5.5k
make ClusterInfo::traffic_stats_ a unique_ptr, so that later we can lazy-init it later. #24406
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
Changes from 9 commits
fba6b7a
803004f
74059e4
fea4a48
b69c02c
9de8e10
58ba476
58bc7d5
150acb0
98c167a
d7bb525
a1dfeb3
c543f83
79ce9bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -765,6 +765,7 @@ MAKE_STATS_STRUCT(ClusterLbStats, ClusterLbStatNames, ALL_CLUSTER_LB_STATS); | |
| */ | ||
| MAKE_STAT_NAMES_STRUCT(ClusterTrafficStatNames, ALL_CLUSTER_TRAFFIC_STATS); | ||
| MAKE_STATS_STRUCT(ClusterTrafficStats, ClusterTrafficStatNames, ALL_CLUSTER_TRAFFIC_STATS); | ||
| using ClusterTrafficStatsPtr = std::unique_ptr<ClusterTrafficStats>; | ||
|
|
||
| MAKE_STAT_NAMES_STRUCT(ClusterLoadReportStatNames, ALL_CLUSTER_LOAD_REPORT_STATS); | ||
| MAKE_STATS_STRUCT(ClusterLoadReportStats, ClusterLoadReportStatNames, | ||
|
|
@@ -1060,9 +1061,12 @@ class ClusterInfo : public Http::FilterChainFactory { | |
| virtual ClusterEndpointStats& endpointStats() const PURE; | ||
|
|
||
| /** | ||
| * @return ClusterTrafficStats& all traffic related stats for this cluster. | ||
| * @return std::unique_ptr<ClusterTrafficStats>& all traffic related stats for this cluster. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you make a nickname ClusterTrafficStatsPtr for the unique_ptr?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're not passing the ownership here, just use raw pointer
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good point, Lizan. However that will not be convenient for the follow-up for this PR. That's because we want to (once we sort out some semantic issues around stat continuity) return a LazyInit& here. I had suggested making this change as an intermediate step because it is a large PR but is (arguably) obviously correct. Then when we have solved stat continuity across xds updates we would be able to pass around the lazy-init reference, and then the first time a stat is incremented, we'll instantiate the block of stats. I think this might appear more palatable once Xin adds a nickname for this.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, if this is an intermediate state just add some comment explaining. A nickname helps as well. |
||
| * NOTE: Returning "std::unique_ptr<ClusterTrafficStats>&" for now to make way for future | ||
| * lazy-init on trafficStats(). See | ||
| * https://github.com/envoyproxy/envoy/pull/23921#issuecomment-1335239116 for more context. | ||
| */ | ||
| virtual ClusterTrafficStats& trafficStats() const PURE; | ||
| virtual std::unique_ptr<ClusterTrafficStats>& trafficStats() const PURE; | ||
|
|
||
| /** | ||
| * @return the stats scope that contains all cluster stats. This can be used to produce dynamic | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,7 +135,7 @@ ConnPoolImplBase::tryCreateNewConnection(float global_preconnect_ratio) { | |
| const bool can_create_connection = host_->canCreateConnection(priority_); | ||
|
|
||
| if (!can_create_connection) { | ||
| host_->cluster().trafficStats().upstream_cx_overflow_.inc(); | ||
| host_->cluster().trafficStats()->upstream_cx_overflow_.inc(); | ||
| } | ||
| // If we are at the connection circuit-breaker limit due to other upstreams having | ||
| // too many open connections, and this upstream has no connections, always create one, to | ||
|
|
@@ -167,15 +167,16 @@ void ConnPoolImplBase::attachStreamToClient(Envoy::ConnectionPool::ActiveClient& | |
| AttachContext& context) { | ||
| ASSERT(client.readyForStream()); | ||
|
|
||
| Upstream::ClusterTrafficStats& traffic_stats = *host_->cluster().trafficStats(); | ||
| if (client.state() == Envoy::ConnectionPool::ActiveClient::State::ReadyForEarlyData) { | ||
| host_->cluster().trafficStats().upstream_rq_0rtt_.inc(); | ||
| traffic_stats.upstream_rq_0rtt_.inc(); | ||
| } | ||
|
|
||
| if (enforceMaxRequests() && !host_->cluster().resourceManager(priority_).requests().canCreate()) { | ||
| ENVOY_LOG(debug, "max streams overflow"); | ||
| onPoolFailure(client.real_host_description_, absl::string_view(), | ||
| ConnectionPool::PoolFailureReason::Overflow, context); | ||
| host_->cluster().trafficStats().upstream_rq_pending_overflow_.inc(); | ||
| traffic_stats.upstream_rq_pending_overflow_.inc(); | ||
| return; | ||
| } | ||
| ENVOY_CONN_LOG(debug, "creating stream", client); | ||
|
|
@@ -185,7 +186,7 @@ void ConnPoolImplBase::attachStreamToClient(Envoy::ConnectionPool::ActiveClient& | |
| client.remaining_streams_--; | ||
| if (client.remaining_streams_ == 0) { | ||
| ENVOY_CONN_LOG(debug, "maximum streams per connection, start draining", client); | ||
| host_->cluster().trafficStats().upstream_cx_max_requests_.inc(); | ||
| traffic_stats.upstream_cx_max_requests_.inc(); | ||
| transitionActiveClientState(client, Envoy::ConnectionPool::ActiveClient::State::Draining); | ||
| } else if (capacity == 1) { | ||
| // As soon as the new stream is created, the client will be maxed out. | ||
|
|
@@ -202,8 +203,8 @@ void ConnPoolImplBase::attachStreamToClient(Envoy::ConnectionPool::ActiveClient& | |
| num_active_streams_++; | ||
| host_->stats().rq_total_.inc(); | ||
| host_->stats().rq_active_.inc(); | ||
| host_->cluster().trafficStats().upstream_rq_total_.inc(); | ||
| host_->cluster().trafficStats().upstream_rq_active_.inc(); | ||
| traffic_stats.upstream_rq_total_.inc(); | ||
| traffic_stats.upstream_rq_active_.inc(); | ||
| host_->cluster().resourceManager(priority_).requests().inc(); | ||
|
|
||
| onPoolReady(client, context); | ||
|
|
@@ -216,7 +217,7 @@ void ConnPoolImplBase::onStreamClosed(Envoy::ConnectionPool::ActiveClient& clien | |
| state_.decrActiveStreams(1); | ||
| num_active_streams_--; | ||
| host_->stats().rq_active_.dec(); | ||
| host_->cluster().trafficStats().upstream_rq_active_.dec(); | ||
| host_->cluster().trafficStats()->upstream_rq_active_.dec(); | ||
| host_->cluster().resourceManager(priority_).requests().dec(); | ||
| // We don't update the capacity for HTTP/3 as the stream count should only | ||
| // increase when a MAX_STREAMS frame is received. | ||
|
|
@@ -282,7 +283,7 @@ ConnectionPool::Cancellable* ConnPoolImplBase::newStreamImpl(AttachContext& cont | |
| ENVOY_LOG(debug, "max pending streams overflow"); | ||
| onPoolFailure(nullptr, absl::string_view(), ConnectionPool::PoolFailureReason::Overflow, | ||
| context); | ||
| host_->cluster().trafficStats().upstream_rq_pending_overflow_.inc(); | ||
| host_->cluster().trafficStats()->upstream_rq_pending_overflow_.inc(); | ||
| return nullptr; | ||
| } | ||
|
|
||
|
|
@@ -490,7 +491,7 @@ void ConnPoolImplBase::onConnectionEvent(ActiveClient& client, absl::string_view | |
|
|
||
| if (!client.hasHandshakeCompleted()) { | ||
| client.has_handshake_completed_ = true; | ||
| host_->cluster().trafficStats().upstream_cx_connect_fail_.inc(); | ||
| host_->cluster().trafficStats()->upstream_cx_connect_fail_.inc(); | ||
| host_->stats().cx_connect_fail_.inc(); | ||
|
|
||
| onConnectFailed(client); | ||
|
|
@@ -595,7 +596,7 @@ void ConnPoolImplBase::onConnectionEvent(ActiveClient& client, absl::string_view | |
| client.currentUnusedCapacity()); | ||
| // No need to update connecting capacity and connect_timer_ as the client is still connecting. | ||
| ASSERT(client.state() == ActiveClient::State::Connecting); | ||
| host()->cluster().trafficStats().upstream_cx_connect_with_0_rtt_.inc(); | ||
| host()->cluster().trafficStats()->upstream_cx_connect_with_0_rtt_.inc(); | ||
| transitionActiveClientState(client, (client.currentUnusedCapacity() > 0 | ||
| ? ActiveClient::State::ReadyForEarlyData | ||
| : ActiveClient::State::Busy)); | ||
|
|
@@ -606,13 +607,14 @@ void ConnPoolImplBase::onConnectionEvent(ActiveClient& client, absl::string_view | |
|
|
||
| PendingStream::PendingStream(ConnPoolImplBase& parent, bool can_send_early_data) | ||
| : parent_(parent), can_send_early_data_(can_send_early_data) { | ||
| parent_.host()->cluster().trafficStats().upstream_rq_pending_total_.inc(); | ||
| parent_.host()->cluster().trafficStats().upstream_rq_pending_active_.inc(); | ||
| Upstream::ClusterTrafficStats& traffic_stats = *parent_.host()->cluster().trafficStats(); | ||
| traffic_stats.upstream_rq_pending_total_.inc(); | ||
| traffic_stats.upstream_rq_pending_active_.inc(); | ||
| parent_.host()->cluster().resourceManager(parent_.priority()).pendingRequests().inc(); | ||
| } | ||
|
|
||
| PendingStream::~PendingStream() { | ||
| parent_.host()->cluster().trafficStats().upstream_rq_pending_active_.dec(); | ||
| parent_.host()->cluster().trafficStats()->upstream_rq_pending_active_.dec(); | ||
| parent_.host()->cluster().resourceManager(parent_.priority()).pendingRequests().dec(); | ||
| } | ||
|
|
||
|
|
@@ -630,7 +632,7 @@ void ConnPoolImplBase::purgePendingStreams( | |
| while (!pending_streams_to_purge_.empty()) { | ||
| PendingStreamPtr stream = | ||
| pending_streams_to_purge_.front()->removeFromList(pending_streams_to_purge_); | ||
| host_->cluster().trafficStats().upstream_rq_pending_failure_eject_.inc(); | ||
| host_->cluster().trafficStats()->upstream_rq_pending_failure_eject_.inc(); | ||
| onPoolFailure(host_description, failure_reason, reason, stream->context()); | ||
| } | ||
| } | ||
|
|
@@ -683,7 +685,7 @@ void ConnPoolImplBase::onPendingStreamCancel(PendingStream& stream, | |
| } | ||
| } | ||
|
|
||
| host_->cluster().trafficStats().upstream_rq_cancelled_.inc(); | ||
| host_->cluster().trafficStats()->upstream_rq_cancelled_.inc(); | ||
| checkForIdleAndCloseIdleConnsIfDraining(); | ||
| } | ||
|
|
||
|
|
@@ -757,16 +759,16 @@ ActiveClient::ActiveClient(ConnPoolImplBase& parent, uint32_t lifetime_stream_li | |
| concurrent_stream_limit_(translateZeroToUnlimited(concurrent_stream_limit)), | ||
| connect_timer_(parent_.dispatcher().createTimer([this]() { onConnectTimeout(); })) { | ||
| conn_connect_ms_ = std::make_unique<Stats::HistogramCompletableTimespanImpl>( | ||
| parent_.host()->cluster().trafficStats().upstream_cx_connect_ms_, | ||
| parent_.host()->cluster().trafficStats()->upstream_cx_connect_ms_, | ||
| parent_.dispatcher().timeSource()); | ||
| conn_length_ = std::make_unique<Stats::HistogramCompletableTimespanImpl>( | ||
| parent_.host()->cluster().trafficStats().upstream_cx_length_ms_, | ||
| parent_.host()->cluster().trafficStats()->upstream_cx_length_ms_, | ||
| parent_.dispatcher().timeSource()); | ||
| connect_timer_->enableTimer(parent_.host()->cluster().connectTimeout()); | ||
| parent_.host()->stats().cx_total_.inc(); | ||
| parent_.host()->stats().cx_active_.inc(); | ||
| parent_.host()->cluster().trafficStats().upstream_cx_total_.inc(); | ||
| parent_.host()->cluster().trafficStats().upstream_cx_active_.inc(); | ||
| parent_.host()->cluster().trafficStats()->upstream_cx_total_.inc(); | ||
| parent_.host()->cluster().trafficStats()->upstream_cx_active_.inc(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. capture
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| parent_.host()->cluster().resourceManager(parent_.priority()).connections().inc(); | ||
| } | ||
|
|
||
|
|
@@ -778,15 +780,15 @@ void ActiveClient::releaseResourcesBase() { | |
|
|
||
| conn_length_->complete(); | ||
|
|
||
| parent_.host()->cluster().trafficStats().upstream_cx_active_.dec(); | ||
| parent_.host()->cluster().trafficStats()->upstream_cx_active_.dec(); | ||
| parent_.host()->stats().cx_active_.dec(); | ||
| parent_.host()->cluster().resourceManager(parent_.priority()).connections().dec(); | ||
| } | ||
| } | ||
|
|
||
| void ActiveClient::onConnectTimeout() { | ||
| ENVOY_CONN_LOG(debug, "connect timeout", *this); | ||
| parent_.host()->cluster().trafficStats().upstream_cx_connect_timeout_.inc(); | ||
| parent_.host()->cluster().trafficStats()->upstream_cx_connect_timeout_.inc(); | ||
| timed_out_ = true; | ||
| close(); | ||
| } | ||
|
|
@@ -811,7 +813,7 @@ void ActiveClient::onConnectionDurationTimeout() { | |
| } | ||
|
|
||
| ENVOY_CONN_LOG(debug, "max connection duration reached, start draining", *this); | ||
| parent_.host()->cluster().trafficStats().upstream_cx_max_duration_reached_.inc(); | ||
| parent_.host()->cluster().trafficStats()->upstream_cx_max_duration_reached_.inc(); | ||
| parent_.transitionActiveClientState(*this, Envoy::ConnectionPool::ActiveClient::State::Draining); | ||
|
|
||
| // Close out the draining client if we no longer have active streams. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.