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: emit a labeled gauge of connected xDS streams by version #10243

Merged
merged 4 commits into from
May 14, 2021

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented May 14, 2021

Fixes #10099

@rboyer rboyer added this to the 1.10.0 milestone May 14, 2021
@rboyer rboyer requested a review from a team May 14, 2021 15:09
@rboyer rboyer self-assigned this May 14, 2021
@vercel vercel bot temporarily deployed to Preview – consul May 14, 2021 15:10 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging May 14, 2021 15:10 Inactive
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM, couple small some suggestions, nothing blocking

agent/xds/xds_protocol_helpers_test.go Show resolved Hide resolved
@@ -647,3 +660,23 @@ func runStep(t *testing.T, name string, fn func(t *testing.T)) {
t.FailNow()
}
}

func requireProtocolVersionGauge(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you expect that we will call this from other places? Right now it's only called from one place so it might be good to keep it next to the one call site.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's called in two places: server_test.go and delta_test.go for v2 and v3 respectively.

agent/xds/server.go Outdated Show resolved Hide resolved
@hashicorp-ci
Copy link
Contributor

🤔 This PR has changes in the website/ directory but does not have a type/docs-cherrypick label. If the changes are for the next version, this can be ignored. If they are updates to current docs, attach the label to auto cherrypick to the stable-website branch after merging.

@vercel vercel bot temporarily deployed to Preview – consul May 14, 2021 18:38 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging May 14, 2021 18:38 Inactive
@rboyer rboyer merged commit ede14b7 into master May 14, 2021
@rboyer rboyer deleted the xds-version-gauge branch May 14, 2021 18:59
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/369442.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit ede14b7 onto release/1.10.x succeeded!

Copy link
Contributor

@markan markan left a comment

Choose a reason for hiding this comment

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

Other than that one question, looks good to me

@@ -141,6 +150,36 @@ type Server struct {
AuthCheckFrequency time.Duration

DisableV2Protocol bool

Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct in assuming that there can only be one Server instance per host? Do we run multiple servers on different ports or anything like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is exactly one xDS server per consul agent.

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.

add simple gauge metric in the xds.Server for number of attached xds streams broken down by v2/v3
5 participants