Skip to content

config: add init fetch timeout stat#7822

Merged
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/init_timeout
Aug 6, 2019
Merged

config: add init fetch timeout stat#7822
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/init_timeout

Conversation

@ramaraochavali
Copy link
Contributor

Description: Adds a stat for init fetch timeout
Risk Level: Low
Testing: Added
Docs Changes: Updated
Release Notes: Added

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Contributor Author

@mattklein123 can you PTAL?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@mattklein123 mattklein123 self-assigned this Aug 5, 2019
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, super useful change. One doc comment.

/wait

:widths: 1, 1, 2

config_reload, Counter, Total API fetches that resulted in a config reload due to a different config
init_fetch_timeout, Counter, Total :ref:`initial fetch timeouts <envoy_api_field_core.ConfigSource.initial_fetch_timeout>`
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, this is a really useful change. However, I think with all of our xDS, it's going to be hard to keep each doc spot up to date. WDYT about maybe have a central doc for "common subscription stats" which we can link to from all the various xDS documentation? cc @htuch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I can add a central doc for "common subscription stats" - I can do a follow-up if it is easier to review the just doc part or if you think it is better to do it as part of this, I am fine with that too. LMK what you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to do it as part of this change as it's a pretty small change. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

Yep, good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattklein123 added the subscription_stats page and linked every where else. PTAL.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks this is great. Small change request. Also, it seems clear that we are missing docs on a bunch of other xDS APIs most likely... cc @htuch

/wait

Subscription statistics
=======================

The following statistics are generated for all subscriptions.
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify what a "subscription" is and maybe cross link to relevant xDS/APIv2 docs? Maybe the title of this should be xDS subscription? WDYT? (Basically I think to the casual reader it won't be clear what this page is about)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will do.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Contributor Author

@mattklein123 changed to xds subscription, clarified what subscription with cross reference. PTAL.
Regarding, docs on a bunch of other xDS APIs , yeah it seems we are missing these stats docs for many xDS apis.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@mattklein123 mattklein123 merged commit 7e5e0b5 into envoyproxy:master Aug 6, 2019
@ramaraochavali ramaraochavali deleted the fix/init_timeout branch August 7, 2019 03:33
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