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.

Sign up for GitHub

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

Jump to bottom

Update ingress/terminating gateway stats labeling #10404

Merged
merged 6 commits into from
Jun 15, 2021
Merged

Update ingress/terminating gateway stats labeling #10404

merged 6 commits into from
Jun 15, 2021

Conversation

freddygv
Copy link
Contributor

@freddygv freddygv commented Jun 15, 2021 •
edited
Loading

Fixes: #9887

This PR updates the stats_prefix for ingress and terminating gateways.

  1. For ingress gateways the change makes it so that the listener port (listenerKey.RouteName()) does not get encoded in the metric name. (Brought up in the issue)
  2. For terminating gateways we also make it so that the listener name does not get encoded in the metric name. There was also a change to the filter name so that the rules defined here apply. This will enable per-destination listener metrics.

These changes are NOT backwards compatible.

Example from issue:

In 1.9.x:

# envoy stat: http.ingress_upstream.14146.no_route: 0
envoy_http_14146_no_route{
  local_cluster="ingress-service",
  consul_source_service="ingress-service",
  consul_source_namespace="default",
  consul_source_datacenter="dc1",
  envoy_http_conn_manager_prefix="ingress_upstream"
} 0

envoy_tcp_default_web_default_downstream_cx_no_route{
  local_cluster="terminating-gateway
  consul_source_service="terminating-gateway
  consul_source_namespace="default
  consul_source_datacenter="dc1
  envoy_tcp_prefix="terminating_gateway"
} 0

Proposed in PR:

envoy_http_no_route{
  local_cluster="ingress-service",
  consul_source_service="ingress-service",
  consul_source_namespace="default",
  consul_source_datacenter="dc1",
  envoy_http_conn_manager_prefix="ingress_upstream_14146"
} 0

envoy_tcp_downstream_cx_no_route{
  local_cluster="terminating-gateway",
  consul_source_service="terminating-gateway",
  consul_source_namespace="default",
  consul_source_datacenter="dc1",
  consul_upstream_service="web",
  consul_upstream_datacenter="dc1",
  consul_upstream_namespace="default",
  envoy_tcp_prefix="upstream"
} 0

freddygv added 3 commits June 15, 2021 08:52
@freddygv
Update terminating gateway stats labeling
6f8c604 
This change makes it so that the stat prefix for terminating gateways
matches that of connect proxies. By using the structure of
"upstream.svc.ns.dc" we can extract labels for the destination service,
namespace, and datacenter.
@freddygv
Update ingress gateway stats labeling
0aec676 
In the absence of stats_tags to handle this pattern, when we pass
"ingress_upstream.$port" as the stat_prefix, Envoy splits up that prefix
and makes the port a part of the metric name.

For example:
- stat_prefix: ingress_upstream.8080

This leads to metric names like envoy_http_8080_no_route. Changing the
stat_prefix to ingress_upstream_80880 yields the expected metric names
such as envoy_http_no_route.

Note that we don't encode the destination's name/ns/dc in this
stat_prefix because for HTTP services ingress gateways use a single
filter chain. Only cluster metrics are available on a per-upstream
basis.
@freddygv
Remove unused param
f3e4705
@freddygv freddygv added this to the 1.10.0 milestone Jun 15, 2021
@freddygv freddygv requested a review from a team June 15, 2021 17:23
@github-actions github-actions bot added the theme/envoy/xds Related to Envoy support label Jun 15, 2021
rboyer
rboyer reviewed Jun 15, 2021
View reviewed changes
...t/xds/testdata/listeners/terminating-gateway-custom-and-tagged-addresses.envoy-1-18-x.golden Outdated
@@ -32,7 +32,7 @@
"name": "envoy.filters.network.tcp_proxy",
"typedConfig": {
"@type": "type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy",
"statPrefix": "terminating_gateway.default.api.foo",
"statPrefix": "upstream.api.default.dc1.",
Copy link
Member

Choose a reason for hiding this comment

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

is this supposed to have a trailing dot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't look like it. Just tested this without it and got the same metrics names.

rboyer
rboyer reviewed Jun 15, 2021
View reviewed changes
agent/xds/listeners.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 15, 2021 20:09 Inactive
@vercel vercel bot temporarily deployed to Preview – consul June 15, 2021 20:09 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 15, 2021 20:15 Inactive
@vercel vercel bot temporarily deployed to Preview – consul June 15, 2021 20:15 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 15, 2021 20:18 Inactive
@vercel vercel bot temporarily deployed to Preview – consul June 15, 2021 20:18 Inactive
rboyer
rboyer approved these changes Jun 15, 2021
View reviewed changes
Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

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

LGTM

@freddygv freddygv merged commit ae88613 into master Jun 15, 2021
@freddygv freddygv deleted the ingress-stats branch June 15, 2021 20:28
@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/387552.

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

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

hc-github-team-consul-core pushed a commit that referenced this pull request Jun 15, 2021
@freddygv @hc-github-team-consul-core
Merge pull request #10404 from hashicorp/ingress-stats
fc86420
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rboyer rboyer rboyer approved these changes

Assignees
No one assigned
Labels
theme/envoy/xds Related to Envoy support
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

Ingress and terminating gateway stats are misnamed since 1.9
3 participants
@freddygv @hc-github-team-consul-core @rboyer