Support Custom Metric Attributes Per Request #6281
Conversation
krak3n
force-pushed
the
feat/dynamic-grpc-labels
branch
from
370d0b2
to
737f700
Compare
|
See also #6092 (which is still a draft). |
|
Could we follow the same pattern as we're doing for otelhttp? |
krak3n
force-pushed
the
feat/dynamic-grpc-labels
branch
from
737f700
to
e20e5cf
Compare
|
@dmathieu pushed up changes. Had problems getting the |
|
@dmathieu would the ability to access the We have a use case where we would only know what attributes to add to the metrics while we are in the handler after db / network calls. |
|
I don't know if we should make metrics so permissive. By their nature, metrics must be low cardinality. |
|
Yeah that's fair @dmathieu. How's this PR looking in general? |
| if f := c.MetricAttributesFn; f != nil { | ||
| attrs := f(ctx, rs.Payload) | ||
|
|
||
| gctx.metricAttrs = append(gctx.metricAttrs, attrs...) |
There was a problem hiding this comment.
It doesn't seem like we need to modify gctx here?
This also causes a race problem.
There was a problem hiding this comment.
So the #6092 also modified this, otherwise the metric attributes won't be propagated on the *stats.End call where the majority of the metrics are recorded, since I don't think *stats.End has access to the Payload although I will check that.
There was a problem hiding this comment.
InPayload is the only stats.RPCStats implementation that has access to the request payload. So the this is only time we can call the MetricAttributesFn to be able to append too gRPCContext.metricAttrs, I can put a lock around metricAttrs protect against races?
There was a problem hiding this comment.
I'm very hesitant about updating a pointer to struct that is stored as a context value. That really looks like a smell.
Unfortunately, we can't do things cleanly (provide a new context with the updated value), as we can't provide a new context as return value.
There was a problem hiding this comment.
Ok, this doesn't seem like it's going to be possible then due to the limitations in how the stats.Handler works.
There was a problem hiding this comment.
@dmathieu any thoughts on if we should continue with this or not?
There was a problem hiding this comment.
I'd like to have other opinions.
There was a problem hiding this comment.
Ok, I'll leave it with you @dmathieu 👍
I did, but I only had issues with one test as I mentioned here: #6281 (comment) |
krak3n
changed the title
|
Coming back at this, I don't think it's a good thing to add. Metrics should be low cardinality, and this really opens a big window for folks to shoot them in the foot with high cardinality metrics. Unless there are objections, I will close this PR in 24 hours. |
|
IMHO developers should decide if the increased cardinality is justified or not. Maybe I'm doing it wrong but in my experience it's much cheaper to add an attribute to a metric than trace every request if you want a comprehensive view of your requests. There's an issue and 2 PRs now trying to solve this so there is clearly demand for the capability. I've currently forked this statshandler in my code to add an extra attribute which is less than ideal. |
|
What attributes are you adding? |
These are domain/service specific attributes in the context. In the case of our monitoring platform (Datadog) we still has the ability to choose to exclude tags from being indexed, so the cardinality and cost increase can be managed quite easily. OTOH enabling tracing for every request to use trace metric is prohibitively expensive at scale, hence the preference to do this as a metric attribute. |
If there are domain/service specific attributes than maybe you can create domain/service specific metrics that would use these attributes instead asking to add custom attributes to instrumentation libraries which are supposed to follow the OpenTelemetry Semantic Conventions.
There may be other ways addressing your use case. |
IIUC you're saying that RPC metrics must never have any attributes other than those listed in the semantic conventions?
Yes recommendations are welcome, I believe these current set of PRs exist because the view was that this was an acceptable solution since it was also done for the http handler. |
I do understand that there are use cases where users may want to add custom attributes. E.g. see: open-telemetry/opentelemetry-specification#4298. However, I am not sure if these capabilities (to add explicitly add additional attributes) should be included to the instrumentation libraries. It may be an SDK capability. I would not say must never, but I would just avoid if possible. Nothing in OpenTelemetry Specification says that instrumentation libraries should offer such capability. Do you know if any other languages (e.g. Java, .NET, Python) allow adding custom metrics to the instrumentation libraries? As far as I remember it is not possible in .NET.
I am not sure if this is actually good that it was added. Notice that |
|
Appreciate the response @pellared We seem to already have a few ways to add metric attributes:
Some making a clear distinction as to why per-request metric attributes are a bridge to far vs resource-wide attributes would help the case for closing this PR and issue. |
What
Adds a new
WithMetricsAttributesFnoption which allows custom metric attributes to be added on a per request basis.Why
So standard metrics provided by the gRPC instrumentation package can be annotated on a per request basis. Our use case is to add attributes to the instruments depending on what the handler is doing with the request.