Add source metadata RFC

[adleong]

When a meshed pod sends outgoing HTTP requests the Linkerd proxy records metrics
such as latency and request counters and scopes those metrics by the
destination. This is done by setting a dst_X label on the Prometheus metrics
where X is the destination workload kind. On the other hand, when a meshed
pod receives incoming HTTP requests, there is no equivalent scoping of the
metrics by source. In other words, there is no src_X Prometheus label, making
it impossible to break down metrics for incoming traffic by source. This RFC
proposes adding src_X Prometheus labels for incoming HTTP traffic.

Signed-off-by: Alex Leong [email protected]

[siggy]

To confirm my own understanding, this proposes to modify all request/response metrics exported via the linkerd-proxy's /metrics endpoint to include the following label keys:

• direction
• src_workload_kind
• src_workload_name
• dst_workload_kind
• dst_workload_name

This implies three changes to the proxy /metrics endpoint:

• adding src_* labels to inbound request/response metrics
• adding dst_* labels to inbound request/response metrics
• adding src_* labels to outbound request/response metrics

Context: linkerd/linkerd2#4101, linkerd/linkerd2#4102

[olix0r]

• Are src labels added to outbound metrics?
• Are dst labels added to inbound metrics?

@siggy note that it won't be all metrics. it will be metrics for all meshed communication. (We can't have dst metrics for services that bypass discovery).

[siggy]

A few comments but overall this is awesome! My reading of this is that it will solve both linkerd/linkerd2#4101 and linkerd/linkerd2#4102.

[adleong]

@siggy @olix0r Yes, both inbound and outbound will have both dst and src metrics. For inbound, the dst_X labels will be set to the pod's own workload (ie the labels loaded from the local file) and for outbound the src_X labels will be set to the pod's own workload.

I tried to make this explicit in the text of the design proposal and in the examples contained therein. If you have any suggestions for how I can make this more clear, please let me know.

[grampelberg]

The non-mixer istio actually does this differently still. Have you taken a peek at how they're doing it now?

[adleong]

I haven't. I'll check it out.

[olix0r]

So, I understand this limitation. But can we be clearer about the problem here? What specific features / use cases are we not able to satisfy without these metrics?

Especially because this proposal only includes this metadata on meshed connections (so we're guaranteed to have the client metrics).

What do these labels enable, concretely, that we can't do otherwise? Or if we do not add them, what risks do we incur?

[adleong]

The next paragraph talks about the limitations we face from not having these metrics.

[olix0r]

So? Is the problem "this is awkward?" or are there fundamental limitations that we incur here?

I want to be careful about looking at this benefit versus the cost of the implementation...

[adleong]

I've added some explicit examples of questions that we can't answer without source metadata.

[olix0r]

I don't think that this RFC actually addresses this issue... We already have client IDs on server metrics.

[adleong]

True, though we don't use that data for this purpose today. We could use it, but it would be more straightforward to use source metadata. While this would not impact consumers of the API, I still think that having a cleaner implementation is of some value.

However, I have removed this point for now.

[olix0r]

This is still a bit squishy... It's difficult, but we do it. I.e. for consumers of our API, there's no overhead. Right?

[adleong]

We do not present traffic metrics in a consistent way today. We're forced to either hide the distinction between server-side and client-side metrics, which is deceitful, or be explicit that certain queries are measured on one side while other queries are measured on the other, which is confusing. Today we split the difference by being vague. So I see this a problem for consumers of the API.

[olix0r]

To put a finer point on this... there's a specific API call for which we return incorrect data, right? What is that API Call?

I think this may be the only problem we're trying to address with this proposal. I'd try to take out much of the other context about labels and asymmetry and focus very concretely on this issue.

[adleong]

I don't think that's quite right. I don't think any of our API calls return incorrect data, their behavior is just confusing and non-uniform.

[olix0r]

This makes it sounds like its our metrics API that is the problem: that we have no way to distinguish these concepts at all.

To put it another way: if we made the changes proposed here, what impact would that have on our metrics API? Would we just subtly change the semantics of an existing call? Would we be unable to represent this call?

I view Linkerd's prometheus data as diagnostics... like log lines, basically. The format & details may change between releases as long as we satisfy our metrics API, which powers consumers within Linkerd. So, a problem statement phrased in terms of our prometheus data isn't all that compelling.

However, it absolutely does sound like a problem if there are fundamental questions that our API cannot answer about mesh traffic. How do we improve Linkerd's metrics API so that it can properly differentiate client and serverside metrics? If this requires source labels, great, I'm all in.

[adleong]

Reworked the RFC to be API focused.

[olix0r]

Love the updated framing.

If it's alright with you, I'm going to suggest some further edits via PR to contextualize how I see this all fitting together.

[olix0r]

This was accidentally closed when we changed the default branch to main. The base should probably be updated...

[siggy]

Noting that as we rethink our metrics and labeling, a nice goal would be to conform to OpenMetrics. One notable requirement is that all metrics with the same name (a MetricsFamily) have the same set of label names:
https://github.com/OpenObservability/OpenMetrics/blob/defac23507e1e78871f8de2648bd809427a1063d/specification/OpenMetrics.md#metric
https://github.com/prometheus/docs/blob/be5550b3759db4d134a9eea9b1a91de092dcc5a8/content/docs/instrumenting/writing_clientlibs.md#labels

Another goal should be to avoid missing metrics. If we start counting requests_total{workload_name="foo"}, we should initialize failed_requests_total{workload_name="foo"}:
https://github.com/prometheus/docs/blob/be5550b3759db4d134a9eea9b1a91de092dcc5a8/content/docs/practices/instrumentation.md#avoid-missing-metrics

More best practices:
https://prometheus.io/docs/instrumenting/writing_exporters/

[sdhoward]

I'm just here to remind that people are still interested in this.