8000 proxy: Add client_id label to tcp_open_total metric · Issue #5031 · linkerd/linkerd2 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

proxy: Add client_id label to tcp_open_total metric #5031

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
Closed
alpeb opened this issue Sep 30, 2020 · 5 comments · Fixed by linkerd/linkerd2-proxy#687
Closed

proxy: Add client_id label to tcp_open_total metric #5031

alpeb opened this issue Sep 30, 2020 · 5 comments · Fixed by linkerd/linkerd2-proxy#687
Assignees
@hawkw
Labels
area/proxy area/telemetry priority/P0 Release Blocker

Comments

@alpeb
Copy link
Member
alpeb commented Sep 30, 2020

In order to have linkerd edges return non-empty values for a connection's CLIENT_ID in the case of TCP connections, the proxy's tcp_open_total metric needs to include the client_id label for inbound connections, like the request_total one for http connections does.

Ref #4999
@olix0r
Copy link
Member
olix0r commented Oct 1, 2020

Ah yeah, for inbound tcp metrics we only seem to include a boolean.

tcp_open_total{peer="src",direction="inbound",tls="true"} 9

@hawkw
Copy link
8000 Contributor
hawkw commented Oct 1, 2020

Okay, I've added client_id labels to the tcp_open_total metric, but linkerd edges is still not showing TCP (MySQL) connections. Is that expected --- does this also require a CLI change to actually consume this data?

@hawkw
Copy link
Contributor
hawkw commented Oct 1, 2020

(I realize this issue never actually says "adding this label will make TCP connections show up in linkerd edges, so I may have misinterpreted it a bit...)

@hawkw hawkw mentioned this issue Oct 1, 2020
Merged
@alpeb
Copy link
Member Author
alpeb commented Oct 1, 2020

Thanks @hawkw , this still requires a change in the control plane. I can take care of that. Can you push a proxy image I can use with this change?

@hawkw
Copy link
Contributor
hawkw commented Oct 1, 2020

Thanks @hawkw , this still requires a change in the control plane. I can take care of that. Can you push a proxy image I can use with this change?

mycoliza/l2-proxy:dev-3e7430d4-eliza (on DockerHub)

olix0r pushed a commit to linkerd/linkerd2-proxy that referenced this issue Oct 1, 2020
@hawkw
metrics: add peer identities to all TLS metric labels (#687)
86c71ea 
In order to have `linkerd edges` return non-empty values for a raw TCP
connection's `CLIENT_ID`, the proxy's `tcp_open_total` metric needs to
include the `client_id` label for inbound connections, like the
`request_total` metrics for http connections does.

This PR changes the `TlsStatus` metric label type to include a peer
identity in the `Conditional::Some` case, rather than `()`. This means
that all metrics with TLS labels will now include the peer identity as
a label.

I've manually verified that this works by running Linkerd locally and
scraping the metrics:

For example, here's an excerpt from Grafana:
```
tcp_open_total{peer="src",direction="inbound",tls="no_identity",no_tls_reason="no_tls_from_remote"} 44
tcp_open_total{peer="dst",direction="inbound",tls="no_identity",no_tls_reason="loopback"} 2
tcp_open_total{peer="src",direction="inbound",tls="true",client_id="linkerd-prometheus.linkerd.serviceaccount.identity.linkerd.cluster.local"}
1
```
And from Prometheus
```
tcp_open_total{peer="dst",authority="10.42.0.25:4191",direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-grafana",dst_namespace="linkerd",dst_pod="linkerd-grafana-65597cf467-vq456",dst_pod_template_hash="65597cf467",dst_serviceaccount="linkerd-grafana",tls="true",server_id="linkerd-grafana.linkerd.serviceaccount.identity.linkerd.cluster.local"} 1
tcp_open_total{peer="dst",authority="10.42.0.25:3000",direction="outbound",dst_control_plane_ns="linkerd",dst_deployment="linkerd-grafana",dst_namespace="linkerd",dst_pod="linkerd-grafana-65597cf467-vq456",dst_pod_template_hash="65597cf467",dst_serviceaccount="linkerd-grafana",tls="true",server_id="linkerd-grafana.linkerd.serviceaccount.identity.linkerd.cluster.local"} 1
```

I'd like to have automated tests for this, but I'd prefer to not have to
write them in the integration style, and use the isolated mock service
style instead. So, tests can be added once #658 lands.

Refs: linkerd/linkerd2#4999
Fixes: linkerd/linkerd2#5031
@olix0r olix0r mentioned this issue Oct 2, 2020
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@hawkw hawkw

Labels
area/proxy area/telemetry priority/P0 Release Blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

metrics: add peer identities to all TLS metric labels linkerd/linkerd2-proxy
3 participants
@olix0r @alpeb @hawkw
0