Skip to content

Commit

Permalink
Revert "api: Add total_issued_requests to Upstream Locality and Endpo…
Browse files Browse the repository at this point in the history 
…int Stats. (envoyproxy#6692)" (envoyproxy#6761)

This reverts commit cdddf54.

Signed-off-by: Lizan Zhou <[email protected]>
  • Loading branch information
@lizan @mattklein123
lizan authored and mattklein123 committed May 1, 2019
1 parent f129e5e commit b748ab4
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 32 deletions.
32 changes: 21 additions & 11 deletions api/envoy/api/v2/endpoint/load_report.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@ message UpstreamLocalityStats {
// collected from. Zone and region names could be empty if unknown.
core.Locality locality = 1;

// The total number of requests sent by this Envoy since the last report. This
// information is aggregated over all the upstream Endpoints. total_requests
// can be inferred from:
//
// .. code-block:: none
//
// total_requests = total_successful_requests + total_requests_in_progress +
// total_error_requests
//
// The total number of requests successfully completed by the endpoints in the
// locality.
uint64 total_successful_requests = 2;
Expand All @@ -36,11 +45,6 @@ message UpstreamLocalityStats {
// aggregated over all endpoints in the locality.
uint64 total_error_requests = 4;

// The total number of requests that were issued by this Envoy since
// the last report. This information is aggregated over all the
// upstream endpoints in the locality.
uint64 total_issued_requests = 8;

// Stats for multi-dimensional load balancing.
repeated EndpointLoadMetricStats load_metric_stats = 5;

Expand All @@ -62,6 +66,16 @@ message UpstreamEndpointStats {
// endpoint. Envoy will pass this directly to the management server.
google.protobuf.Struct metadata = 6;

// The total number of requests successfully completed by the endpoint. A
// single HTTP or gRPC request or stream is counted as one request. A TCP
// connection is also treated as one request. There is no explicit
// total_requests field below for an endpoint, but it may be inferred from:
//
// .. code-block:: none
//
// total_requests = total_successful_requests + total_requests_in_progress +
// total_error_requests
//
// The total number of requests successfully completed by the endpoints in the
// locality. These include non-5xx responses for HTTP, where errors
// originate at the client and the endpoint responded successfully. For gRPC,
Expand All @@ -83,11 +97,6 @@ message UpstreamEndpointStats {
// - DataLoss
uint64 total_error_requests = 4;

// The total number of requests that were issued to this endpoint
// since the last report. A single TCP connection, HTTP or gRPC
// request or stream is counted as one request.
uint64 total_issued_requests = 7;

// Stats for multi-dimensional load balancing.
repeated EndpointLoadMetricStats load_metric_stats = 5;
}
Expand Down Expand Up @@ -129,7 +138,8 @@ message ClusterStats {
//
// .. code-block:: none
//
// sum_locality(total_issued_requests) + total_dropped_requests`
// sum_locality(total_successful_requests) + sum_locality(total_requests_in_progress) +
// sum_locality(total_error_requests) + total_dropped_requests`
//
// The total number of dropped requests. This covers requests
// deliberately dropped by the drop_overload policy and circuit breaking.
Expand Down
1 change: 0 additions & 1 deletion docs/root/intro/version_history.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ Version history
1.11.0 (Pending)
================
* access log: added a new field for response code details in :ref:`file access logger<config_access_log_format_response_code_details>` and :ref:`gRPC access logger<envoy_api_field_data.accesslog.v2.HTTPResponseProperties.response_code_details>`.
* api: track and report requests issued since last load report.
* dubbo_proxy: support the :ref:`Dubbo proxy filter <config_network_filters_dubbo_proxy>`.
* eds: added support to specify max time for which endpoints can be used :ref:`gRPC filter <envoy_api_msg_ClusterLoadAssignment.Policy>`.
* event: added :ref:`loop duration and poll delay statistics <operations_performance>`.
Expand Down
4 changes: 0 additions & 4 deletions source/common/upstream/load_stats_reporter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,10 @@ void LoadStatsReporter::sendLoadStatsRequest() {
uint64_t rq_success = 0;
uint64_t rq_error = 0;
uint64_t rq_active = 0;
uint64_t rq_issued = 0;
for (auto host : hosts) {
rq_success += host->stats().rq_success_.latch();
rq_error += host->stats().rq_error_.latch();
rq_active += host->stats().rq_active_.value();
rq_issued += host->stats().rq_total_.latch();
}
if (rq_success + rq_error + rq_active != 0) {
auto* locality_stats = cluster_stats->add_upstream_locality_stats();
Expand All @@ -77,7 +75,6 @@ void LoadStatsReporter::sendLoadStatsRequest() {
locality_stats->set_total_successful_requests(rq_success);
locality_stats->set_total_error_requests(rq_error);
locality_stats->set_total_requests_in_progress(rq_active);
locality_stats->set_total_issued_requests(rq_issued);
}
}
}
Expand Down Expand Up @@ -157,7 +154,6 @@ void LoadStatsReporter::startLoadReportPeriod() {
for (auto host : host_set->hosts()) {
host->stats().rq_success_.latch();
host->stats().rq_error_.latch();
host->stats().rq_total_.latch();
}
}
cluster.info()->loadReportStats().upstream_rq_dropped_.latch();
Expand Down
27 changes: 11 additions & 16 deletions test/integration/load_stats_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ class LoadStatsIntegrationTest : public testing::TestWithParam<Network::Address:

envoy::api::v2::endpoint::UpstreamLocalityStats localityStats(const std::string& sub_zone,
uint64_t success, uint64_t error,
uint64_t active, uint64_t issued,
uint64_t active,
uint32_t priority = 0) {
envoy::api::v2::endpoint::UpstreamLocalityStats locality_stats;
auto* locality = locality_stats.mutable_locality();
Expand All @@ -304,7 +304,6 @@ class LoadStatsIntegrationTest : public testing::TestWithParam<Network::Address:
locality_stats.set_total_successful_requests(success);
locality_stats.set_total_error_requests(error);
locality_stats.set_total_requests_in_progress(active);
locality_stats.set_total_issued_requests(issued);
locality_stats.set_priority(priority);
return locality_stats;
}
Expand Down Expand Up @@ -365,8 +364,7 @@ TEST_P(LoadStatsIntegrationTest, Success) {
}

// Verify we do not get empty stats for non-zero priorities.
waitForLoadStatsRequest(
{localityStats("winter", 2, 0, 0, 2), localityStats("dragon", 2, 0, 0, 2)});
waitForLoadStatsRequest({localityStats("winter", 2, 0, 0), localityStats("dragon", 2, 0, 0)});

EXPECT_EQ(1, test_server_->counter("load_reporter.requests")->value());
// On slow machines, more than one load stats response may be pushed while we are simulating load.
Expand All @@ -383,8 +381,7 @@ TEST_P(LoadStatsIntegrationTest, Success) {

// No locality for priority=1 since there's no "winter" endpoints.
// The hosts for dragon were received because membership_total is accurate.
waitForLoadStatsRequest(
{localityStats("winter", 2, 0, 0, 2), localityStats("dragon", 4, 0, 0, 4)});
waitForLoadStatsRequest({localityStats("winter", 2, 0, 0), localityStats("dragon", 4, 0, 0)});

EXPECT_EQ(2, test_server_->counter("load_reporter.requests")->value());
EXPECT_LE(3, test_server_->counter("load_reporter.responses")->value());
Expand All @@ -400,7 +397,7 @@ TEST_P(LoadStatsIntegrationTest, Success) {
}

waitForLoadStatsRequest(
{localityStats("winter", 2, 0, 0, 2, 1), localityStats("dragon", 2, 0, 0, 2, 1)});
{localityStats("winter", 2, 0, 0, 1), localityStats("dragon", 2, 0, 0, 1)});
EXPECT_EQ(3, test_server_->counter("load_reporter.requests")->value());
EXPECT_LE(4, test_server_->counter("load_reporter.responses")->value());
EXPECT_EQ(0, test_server_->counter("load_reporter.errors")->value());
Expand All @@ -414,7 +411,7 @@ TEST_P(LoadStatsIntegrationTest, Success) {
sendAndReceiveUpstream(1);
}

waitForLoadStatsRequest({localityStats("winter", 1, 0, 0, 1)});
waitForLoadStatsRequest({localityStats("winter", 1, 0, 0)});
EXPECT_EQ(4, test_server_->counter("load_reporter.requests")->value());
EXPECT_LE(5, test_server_->counter("load_reporter.responses")->value());
EXPECT_EQ(0, test_server_->counter("load_reporter.errors")->value());
Expand All @@ -427,7 +424,7 @@ TEST_P(LoadStatsIntegrationTest, Success) {
sendAndReceiveUpstream(1);
sendAndReceiveUpstream(1);

waitForLoadStatsRequest({localityStats("winter", 3, 0, 0, 3)});
waitForLoadStatsRequest({localityStats("winter", 3, 0, 0)});

EXPECT_EQ(6, test_server_->counter("load_reporter.requests")->value());
EXPECT_LE(6, test_server_->counter("load_reporter.responses")->value());
Expand All @@ -441,7 +438,7 @@ TEST_P(LoadStatsIntegrationTest, Success) {
sendAndReceiveUpstream(1);
sendAndReceiveUpstream(1);

waitForLoadStatsRequest({localityStats("winter", 2, 0, 0, 2)});
waitForLoadStatsRequest({localityStats("winter", 2, 0, 0)});

EXPECT_EQ(8, test_server_->counter("load_reporter.requests")->value());
EXPECT_LE(7, test_server_->counter("load_reporter.responses")->value());
Expand Down Expand Up @@ -476,8 +473,7 @@ TEST_P(LoadStatsIntegrationTest, LocalityWeighted) {
sendAndReceiveUpstream(0);

// Verify we get the expect request distribution.
waitForLoadStatsRequest(
{localityStats("winter", 4, 0, 0, 4), localityStats("dragon", 2, 0, 0, 2)});
waitForLoadStatsRequest({localityStats("winter", 4, 0, 0), localityStats("dragon", 2, 0, 0)});

EXPECT_EQ(1, test_server_->counter("load_reporter.requests")->value());
// On slow machines, more than one load stats response may be pushed while we are simulating load.
Expand Down Expand Up @@ -510,8 +506,7 @@ TEST_P(LoadStatsIntegrationTest, NoLocalLocality) {
// order of locality stats is different to the Success case, where winter is
// the local locality (and hence first in the list as per
// HostsPerLocality::get()).
waitForLoadStatsRequest(
{localityStats("dragon", 2, 0, 0, 2), localityStats("winter", 2, 0, 0, 2)});
waitForLoadStatsRequest({localityStats("dragon", 2, 0, 0), localityStats("winter", 2, 0, 0)});

EXPECT_EQ(1, test_server_->counter("load_reporter.requests")->value());
// On slow machines, more than one load stats response may be pushed while we are simulating load.
Expand All @@ -538,7 +533,7 @@ TEST_P(LoadStatsIntegrationTest, Error) {
// This should count as "success" since non-5xx.
sendAndReceiveUpstream(0, 404);

waitForLoadStatsRequest({localityStats("winter", 1, 1, 0, 2)});
waitForLoadStatsRequest({localityStats("winter", 1, 1, 0)});

EXPECT_EQ(1, test_server_->counter("load_reporter.requests")->value());
EXPECT_LE(2, test_server_->counter("load_reporter.responses")->value());
Expand All @@ -558,7 +553,7 @@ TEST_P(LoadStatsIntegrationTest, InProgress) {

requestLoadStatsResponse({"cluster_0"});
initiateClientConnection();
waitForLoadStatsRequest({localityStats("winter", 0, 0, 1, 1)});
waitForLoadStatsRequest({localityStats("winter", 0, 0, 1)});

waitForUpstreamResponse(0, 503);
cleanupUpstreamAndDownstream();
Expand Down

0 comments on commit b748ab4

Please sign in to comment.