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

xDS: add version gauge #1805

Merged
merged 6 commits into from
Oct 4, 2017
Merged

xDS: add version gauge #1805

merged 6 commits into from
Oct 4, 2017

Conversation

junr03
Copy link
Member

@junr03 junr03 commented Oct 4, 2017

Surface the version_info field in the xDS subscriptions as a gauge in the xDS stats trees.

Signed-off-by: Jose Nino [email protected]

Signed-off-by: Jose Nino <[email protected]>
@junr03
Copy link
Member Author

junr03 commented Oct 4, 2017

@danielhochman @htuch up for review

@@ -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_));
Copy link
Contributor

Choose a reason for hiding this comment

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

i would just hash the response body again here

Copy link
Member

Choose a reason for hiding this comment

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

Please avoid computing the hash twice. Just return a tuple or something.

Copy link
Member

Choose a reason for hiding this comment

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

But +1 for using same hash.

@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

*http.<stat_prefix>.rds.<route_config_name>.*

@mattklein123
Copy link
Member

Per #1806 please put the RDS per table stats into a dedicated scope, so that any naming fixes are automatically picked up. cc @hennna

Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: Jose Nino <[email protected]>
@junr03
Copy link
Member Author

junr03 commented Oct 4, 2017

@danielhochman @htuch @mattklein123 ready for another pass.

@@ -105,6 +107,7 @@ class HttpSubscriptionImpl : public Http::RestApiFetcher,
}

std::string path_;
std::string version_info_;
Copy link
Contributor

Choose a reason for hiding this comment

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

why add this local var?

std::string RdsRouteConfigProviderImpl::rdsStatsName(const std::string& stat_prefix,
const std::string& route_config_name) {
std::string stats_name = fmt::format("{}rds.{}.", stat_prefix, route_config_name);
std::replace(stats_name.begin(), stats_name.end(), ':', '_');
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a test case for this? cc @hennna please update this site also in your other PR if we move this into scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used all the stats expectations as a test that the name is correct. But I will make an explicit test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've updated #1806. Let me know if I'm missing anything else.

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove this in favor of @hennna's change.

@@ -69,7 +69,9 @@ 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>.*.
Any ``:`` character in the ``route_config_name`` name gets replaces with ``_`` in the
Copy link
Member

Choose a reason for hiding this comment

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

Nit: s/replaces/replaced/

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
Copy link
Member

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?

Copy link
Member Author

@junr03 junr03 Oct 4, 2017

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.

Copy link
Contributor

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.

Jose Nino added 2 commits October 4, 2017 12:21
Signed-off-by: Jose Nino <[email protected]>
@junr03
Copy link
Member Author

junr03 commented Oct 4, 2017

@danielhochman updated

@@ -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) \
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: align the spacing of \

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm I thought fix_format would take care of this.

@@ -235,7 +235,8 @@ class HostSet {
COUNTER(update_attempt) \
COUNTER(update_success) \
COUNTER(update_failure) \
COUNTER(update_empty)
COUNTER(update_empty) \
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: fix spacing of \

Signed-off-by: Jose Nino <[email protected]>
@junr03 junr03 merged commit cff1866 into master Oct 4, 2017
@junr03 junr03 deleted the version-gauge branch October 4, 2017 21:45
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Description: expose the new per try idle timeout via the engine builder.
Risk Level: low
Testing: unit test
Docs Changes: added

Signed-off-by: Jose Nino <[email protected]>

Co-authored-by: Alan Chiu <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Description: expose the new per try idle timeout via the engine builder.
Risk Level: low
Testing: unit test
Docs Changes: added

Signed-off-by: Jose Nino <[email protected]>

Co-authored-by: Alan Chiu <[email protected]>
Signed-off-by: JP Simard <[email protected]>
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.

None yet

6 participants