A89: Backend Service Metric Label #471
Conversation
A89-xds-cluster-metric-label.md
Outdated
| to have a different value. This is the case for locality as well, and the last | ||
| pick's value should be used. | ||
|
|
||
| The `grpc.xds.cluster` label will be available on the following per-call |
There was a problem hiding this comment.
@yashykt, Java hard-codes support for locality today. I remember there being a conversation about supporting arbitrary optional labels, and I think we had kicked that can down the road.
I'm envisioning just adding another hard-coded case to the otel module for this.
A89-xds-cluster-metric-label.md
Outdated
| ## Implementation | ||
|
|
||
| @ejona86 will immediately implement in gRPC Java. Other languages will follow as | ||
| able. The implementation is very quick. |
There was a problem hiding this comment.
"The implementation is very quick" is more rationale than blueprint.
There was a problem hiding this comment.
It is an answer to "where are the resources coming from." "It is quick, so doesn't need a ton of planning."
There was a problem hiding this comment.
I'm not sure that kind of justification is needed for this document. It feels a little out of place in a specification intended for long-term use. But I wouldn't block the PR on it if you feel like it's needed.
I changed the link name so I wouldn't have to specify the link name, and then forgot to remove the link name.
A89-xds-cluster-metric-label.md
Outdated
| ## Implementation | ||
|
|
||
| @ejona86 will immediately implement in gRPC Java. Other languages will follow as | ||
| able. The implementation is very quick. |
There was a problem hiding this comment.
I'm not sure that kind of justification is needed for this document. It feels a little out of place in a specification intended for long-term use. But I wouldn't block the PR on it if you feel like it's needed.
This impacts a lot of the language, but I'll fix that up later.
| The value will be communicated to the gRPC OpenTelemetry module by the call | ||
| attempt tracer. When an LB policy provides the label value to the tracer it | ||
| will do so each pick that the information is available, regardless of the pick's | ||
| result. This allows DEADLINE_EXCEEDED and UNAVAILABLE failures to include a |
There was a problem hiding this comment.
I assume this is talking solely about including this information in metrics, right? Do we already include the locality label for DEADLINE_EXCEEDED and UNAVAILABLE? @yashykt
There was a problem hiding this comment.
Yes, I was meaning in metrics. The current locality implementation can't include this information in erring metrics because xds_cluster_impl is what does the addOptionalLabel() and it only knows xds_wrr_locality's locality decision by looking at the subchannel of a successful pick.
I had some discussion with Yash yesterday about some edge cases and improving things post-A75, just to see how he felt. I was going to bring them up in our Friday meeting. (I just invited Yash to it.)
(Preview: Post-A75, have this logic in the leaf cds policy (the first policy under priority) so we get labels for outlier detection failures. Similarly, we could move the locality handling to weighted_target, or a new policy between xds_wrr_locality/weighted_target and the endpoint-picking policy, to add the locality label for failed picks. The question is mostly "do we care.")
markdroth
changed the title
No description provided.