exporter/prometheus: Expose scope name, version, schema URL and attributes as labels #40004
Conversation
ArthurSens
force-pushed
the
spec#4223-prom-exporter
branch
from
b438ba9 to
affa338
Compare
|
I couldn't find an easy way of accessing SchemaURL, though. The current instrumentation scope used in the exporter doesn't include schema 🤔 |
ArthurSens
force-pushed
the
spec#4223-prom-exporter
branch
from
affa338 to
289e54f
Compare
Instrumentation scope does not have a schema URL, the |
|
@mx-psi According to the spec the schema URL is part of the scope though (definition of instrumentation scope in the glossary). If it's meant to be separate like in the proto, we need to change the spec. |
@jade-guiton-dd I don't think the spec requires the proto representation of the instrumentation scope to be all in a single field (at least I would say the glossary item you linked does not require that). I think the proto is weird but spec-compliant: you can still do a 1:1 association between the schema URL and the rest of the scope fields. |
|
Ah sorry, I badly misinterpreted your comment (I thought you were suggesting we shouldn't expose the schema URL as part of the |
ArthurSens
force-pushed
the
spec#4223-prom-exporter
branch
3 times, most recently
from
29d5322 to
d249acc
Compare
…s as labels Signed-off-by: Arthur Silva Sens <[email protected]>
ArthurSens
force-pushed
the
spec#4223-prom-exporter
branch
from
d249acc to
6e6f5ae
Compare
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
ArthurSens
changed the title
|
|
||
| attrs := make([]string, 0, scopeAttributes.Len()) | ||
| for k, v := range scopeAttributes.All() { | ||
| attrs = append(attrs, k+"*"+v.AsString()) |
There was a problem hiding this comment.
since this uses the same delimiter * as attributes below, it seems possible for there to be a collision between timeseries where an attribute is a scope attribute in one, but a metric attribute in another.
| } | ||
|
|
||
| func timeseriesSignature(ilmName string, metric pmetric.Metric, attributes pcommon.Map, resourceAttrs pcommon.Map) string { | ||
| func timeseriesSignature(scopeName string, scopeVersion string, scopeSchemaURL string, scopeAttributes pcommon.Map, metric pmetric.Metric, attributes pcommon.Map, resourceAttrs pcommon.Map) string { |
There was a problem hiding this comment.
Doesn't block this PR, but we are building massive strings to compute signatures here (contains all resource and metric attributes). That is probably using a huge amount of memory. We should strongly consider using something like internal/exp/metrics/identity.OfResourceMetric instead of computing large strings.
There was a problem hiding this comment.
That's a good point, moving to the identity package would also solve the string collision problem. Would you be okay with me working on those in a follow-up PR? That way I don't need to solve the same problem twice :)
There was a problem hiding this comment.
yes, i'm OK with a follow-up
ArthurSens
added
the
ready to merge
Fixes #4223 Prototypes: - open-telemetry/opentelemetry-go#5947 - open-telemetry/opentelemetry-go#6770 - open-telemetry/opentelemetry-java#7356 - open-telemetry/opentelemetry-collector-contrib#40060 - open-telemetry/opentelemetry-collector-contrib#40004 ## Changes Currently (before this PR) [Prometheus and OpenMetrics Compatibility](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md) assumes that only scope name and scope version are identifying. However, with #4161 this is no longer true. Therefore, this PR updates the Prometheus and OpenMetrics Compatibility specification to add the scope name, version, schema URL, scope attributes to all metrics. This also removes the `otel_scope_info` as it looks that it won't be useful. See: #4223 (comment). This change important for Collector open-telemetry/opentelemetry-go#5846 (comment). It is also is necessary towards stabilization of OTel-Prom/OpenMetrics compatibility) and the Prometheus exporter. _Initially, I thought about [splitting it into a few PRs](#4223 (comment)). However, it looks like doing it in one PR would be a more complete approach (also there are not that many changes)._ --------- Co-authored-by: Jade Guiton <[email protected]> Co-authored-by: Carlos Alberto Cortez <[email protected]>
Fixes #4223 Prototypes: - open-telemetry/opentelemetry-go#5947 - open-telemetry/opentelemetry-go#6770 - open-telemetry/opentelemetry-java#7356 - open-telemetry/opentelemetry-collector-contrib#40060 - open-telemetry/opentelemetry-collector-contrib#40004 ## Changes Currently (before this PR) [Prometheus and OpenMetrics Compatibility](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md) assumes that only scope name and scope version are identifying. However, with #4161 this is no longer true. Therefore, this PR updates the Prometheus and OpenMetrics Compatibility specification to add the scope name, version, schema URL, scope attributes to all metrics. This also removes the `otel_scope_info` as it looks that it won't be useful. See: #4223 (comment). This change important for Collector open-telemetry/opentelemetry-go#5846 (comment). It is also is necessary towards stabilization of OTel-Prom/OpenMetrics compatibility) and the Prometheus exporter. _Initially, I thought about [splitting it into a few PRs](#4223 (comment)). However, it looks like doing it in one PR would be a more complete approach (also there are not that many changes)._ --------- Co-authored-by: Jade Guiton <[email protected]> Co-authored-by: Carlos Alberto Cortez <[email protected]>
Description
This PR is a PoC for the spec change described at open-telemetry/opentelemetry-specification#4223
In the specification change, it's being proposed that Prometheus exporters stop generating the metric
otel_scope_infowith all the scope attributes and instead add all scope information as labels, turning Scope information "identifying".Turns out our exporter never exposed
otel_scope_info, so this PR just introduces the addition of the new labels.