-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
xDS: add version gauge #1805
xDS: add version gauge #1805
Changes from 1 commit
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 |
---|---|---|
|
@@ -69,7 +69,7 @@ will only perform a full reload if the hash value changes. | |
Statistics | ||
---------- | ||
|
||
RDS has a statistics tree rooted at *http.<stat_prefix>.rds.* with the following statistics: | ||
RDS has a statistics tree rooted at *http.<stat_prefix>.rds.<route_config_name>* with the following statistics: | ||
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.
|
||
|
||
.. csv-table:: | ||
:header: Name, Type, Description | ||
|
@@ -79,3 +79,4 @@ RDS has a statistics tree rooted at *http.<stat_prefix>.rds.* with the following | |
update_attempt, Counter, Total API fetches attempted | ||
update_success, Counter, Total API fetches completed successfully | ||
update_failure, Counter, Total API fetches that failed (either network or schema errors) | ||
version, Gauge, Hash of the contents from the last successful API fetch |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,18 +75,19 @@ template <class ResourceType> class Subscription { | |
* Per subscription stats. @see stats_macros.h | ||
*/ | ||
// clang-format off | ||
#define ALL_SUBSCRIPTION_STATS(COUNTER) \ | ||
#define ALL_SUBSCRIPTION_STATS(COUNTER, GAUGE) \ | ||
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. nit: align the spacing of 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. hmm I thought |
||
COUNTER(update_attempt) \ | ||
COUNTER(update_success) \ | ||
COUNTER(update_failure) \ | ||
COUNTER(update_rejected) | ||
COUNTER(update_rejected) \ | ||
GAUGE(version) | ||
// clang-format on | ||
|
||
/** | ||
* Struct definition for per subscription stats. @see stats_macros.h | ||
*/ | ||
struct SubscriptionStats { | ||
ALL_SUBSCRIPTION_STATS(GENERATE_COUNTER_STRUCT) | ||
ALL_SUBSCRIPTION_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT) | ||
}; | ||
|
||
} // namespace Config | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,7 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, | |
request_.mutable_resource_names()->Swap(&resources_vector); | ||
} | ||
|
||
const std::string versionInfo() const override { return request_.version_info(); } | ||
const std::string versionInfo() const override { return version_info_; } | ||
|
||
// Http::RestApiFetcher | ||
void createRequest(Http::Message& request) override { | ||
|
@@ -83,6 +83,8 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, | |
try { | ||
callbacks_->onConfigUpdate(typed_resources); | ||
request_.set_version_info(message.version_info()); | ||
version_info_ = message.version_info(); | ||
stats_.version_.set(HashUtil::xxHash64(version_info_)); | ||
stats_.update_success_.inc(); | ||
} catch (const EnvoyException& e) { | ||
ENVOY_LOG(warn, "REST config update rejected: {}", e.what()); | ||
|
@@ -105,6 +107,7 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher, | |
} | ||
|
||
std::string path_; | ||
std::string version_info_; | ||
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. why add this local var? |
||
Protobuf::RepeatedPtrField<ProtobufTypes::String> resources_; | ||
Config::SubscriptionCallbacks<ResourceType>* callbacks_{}; | ||
envoy::api::v2::DiscoveryRequest request_; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,7 @@ void RdsSubscription::parseResponse(const Http::Message& response) { | |
resources[0].set_name(route_config_name_); | ||
callbacks_->onConfigUpdate(resources); | ||
version_info_ = Envoy::Config::Utility::computeHashedVersion(response_body); | ||
stats_.version_.set(HashUtil::xxHash64(version_info_)); | ||
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. i would just hash the response body again here 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. Please avoid computing the hash twice. Just return a tuple or something. 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. But +1 for using same hash. |
||
stats_.update_success_.inc(); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we also want to report the actual
version_info
as provided by v2 xDS?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can provide that via a gauge, given that version_info is a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could attempt atoul on the first 6 chars similar to the server's version info and fallback to hashing. i'm not sure that it's worth it though.