8000 metrics: add peer identities to all TLS metric labels by hawkw · Pull Request #687 · linkerd/linkerd2-proxy · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

metrics: add peer identities to all TLS metric labels #687

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
Merged
merged 3 commits into from
Oct 1, 2020
Merged

metrics: add peer identities to all TLS metric labels #687

merged 3 commits into from
Oct 1, 2020

Conversation

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

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

hawkw added 3 commits October 1, 2020 11:04
@hawkw
add client & server identity to all metric labels
6c1f866
@hawkw
missing comma
e5bda48 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
whoops, fix both being client ids
b1bc961 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from a team October 1, 2020 21:51
olix0r
olix0r approved these changes Oct 1, 2020
View reviewed changes
Copy link
Member
@olix0r olix0r left a comment

Choose a reason for hiding this comment

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

looks good! thanks

@olix0r olix0r merged commit 86c71ea into main Oct 1, 2020
@olix0r olix0r deleted the eliza/tcp-in-labels branch October 1, 2020 22:55
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Oct 2, 2020
@olix0r
proxy: v2.113.0
2d9d07b 
This release includes changes to TCP metrics to ensure that peer
identities are encoded via the `client_id` and `server_id` labels.

---

* outbound: Explicitly ignore the source address for tap (linkerd/linkerd2-proxy#680)
* Update proxy-api and tonic (linkerd/linkerd2-proxy#682)
* http: Lazily build http/tcp stacks (linkerd/linkerd2-proxy#681)
* outbound: Remove required identity from HttpLogical (linkerd/linkerd2-proxy#683)
* profiles: Expose the fully_qualified_name (linkerd/linkerd2-proxy#684)
* request-filter: Support altering the request type (linkerd/linkerd2-proxy#685)
* tracing: Set contexts in new_service/make_service (linkerd/linkerd2-proxy#686)
* discover: Allow resolution streams to terminate (linkerd/linkerd2-proxy#689)
* metrics: add peer identities to all TLS metric labels (linkerd/linkerd2-proxy#687)
* outbound: Return a default endpoint on reject (linkerd/linkerd2-proxy#690)
* Skip endpoint resolution when profile lookup is rejected (linkerd/linkerd2-proxy#691)
@olix0r olix0r mentioned this pull request Oct 2, 2020
Merged
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Oct 2, 2020
@olix0r
proxy: v2.113.0 (#5037)
127791c 
This release includes changes to TCP metrics to ensure that peer
identities are encoded via the `client_id` and `server_id` labels.

---

* outbound: Explicitly ignore the source address for tap (linkerd/linkerd2-proxy#680)
* Update proxy-api and tonic (linkerd/linkerd2-proxy#682)
* http: Lazily build http/tcp stacks (linkerd/linkerd2-proxy#681)
* outbound: Remove required identity from HttpLogical (linkerd/linkerd2-proxy#683)
* profiles: Expose the fully_qualified_name (linkerd/linkerd2-proxy#684)
* request-filter: Support altering the request type (linkerd/linkerd2-proxy#685)
* tracing: Set contexts in new_service/make_service (linkerd/linkerd2-proxy#686)
* discover: Allow resolution streams to terminate (linkerd/linkerd2-proxy#689)
* metrics: add peer identities to all TLS metric labels (linkerd/linkerd2-proxy#687)
* outbound: Return a default endpoint on reject (linkerd/linkerd2-proxy#690)
* Skip endpoint resolution when profile lookup is rejected (linkerd/linkerd2-proxy#691)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olix0r olix0r olix0r approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

proxy: Add client_id label to tcp_open_total metric
2 participants
@hawkw @olix0r
0