fix(nexus): add nexus service and operation tags to metrics #8149
Conversation
|
We shouldn't add labels to existing metrics without prior discussion, when we added these label on the frontend, we put them behind a dynamic config to avoid high cardinality issues, which can bring down metrics backends. What's the motivation for adding this? |
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
TBH, I would just not add these labels altogether until we see a reason to include them but at this point the work has already been done so we may as well merge it.
Please see my comment below.
| tags = append(tags, metrics.NexusOperationTag(operation)) | ||
| } | ||
|
|
||
| for _, mapping := range conf.HeaderTagMappings { |
There was a problem hiding this comment.
This config was meant for the handler side, I'm a bit hesitant to also apply it on the caller side but I guess there's no harm in doing so.
| metricTagConfig: h.Config.MetricTagConfig, | ||
| } | ||
| ctx = rCtx.augmentContext(ctx, r.HTTPRequest.Header) | ||
| rCtx.enrichNexusOperationMetrics(r.HTTPRequest.Method, r.HTTPRequest.URL.Path, r.HTTPRequest.Header) |
There was a problem hiding this comment.
Those aren't service and operation names. These labels can't be available on this metric.
| PayloadSizeLimit: dynamicconfig.BlobSizeLimitError.Get(coll), | ||
| ForwardingEnabledForNamespace: dynamicconfig.EnableNamespaceNotActiveAutoForwarding.Get(coll), | ||
| MaxOperationTokenLength: nexusoperations.MaxOperationTokenLength.Get(coll), | ||
| MetricTagConfig: dynamicconfig.NewGlobalCachedTypedValue( |
There was a problem hiding this comment.
I'm going to remove this soon (finally). If #7052 lands first then you can just get rid of this and use the setting directly. If this lands first, I'll do it in that PR.
What changed?
added
nexus_{service,operation}tags to metrics for better observabilityWhy?
Metrics were missing these fields making debugging difficult
How did you test it?
Potential risks
NA