Prometheus: preserve job/instance when translating Prometheus -> OTLP - #4956
Prometheus: preserve job/instance when translating Prometheus -> OTLP#4956dashpole wants to merge 10 commits into
Conversation
|
Hmmmm, to make quicker progress here, would it make sense to focus on the Prometheus -> OTLP transformation and leave OTLP -> Prometheus in a separate PR? I feel like OTLP -> Prometheus is a lot more complicated, and will require deeper discussions with other parts of OTel, like Infrastructure semantic conventions, OBI, etc 🤔 |
|
My main goal is to make sure that everything still round-trips properly. If I make changes to one direction, I usually have to make the inverse change to the other direction to ensure that. I can remove some of the editorial changes to the OTLP -> Prometheus section if that helps. But I would like aggregated exporters to accept |
|
This PR was marked stale. It will be closed in 14 days without additional activity. |
ArthurSens
left a comment
There was a problem hiding this comment.
I did a pass through the PR, and so far, I have just one comment.
I feel like I haven't fully grasped the potential consequences of this change, so I'm taking some extra time to reflect, I hope that's ok :)
7248acd to
4dda69a
Compare
ArthurSens
left a comment
There was a problem hiding this comment.
This behavior seems very very similar to the configuration option called keep_identifying_attributes from the Prometheus Server configuration.
I know we're only modifying the data model spec here, not the exporter configuration spec, but I wonder whether we'd like to make adjustments to the exporter spec or the Prometheus configuration so both OTel and Prometheus are aligned.
@aknuds1, I think this is somewhat close to work you've done with @cyrille-leclerc in the past. Do you have anything extra to add?
Yeah. If we went with this change, it would make sense to eventually switch that to be true by default (e.g. in the next major version, if we consider it breaking). It would be less likely to be duplicate information, so it would make more sense for users to keep the original ones. |
|
I'll have a look. |
@ArthurSens @dashpole AFAICT, the following implies
|
This is meant to say that |
aknuds1
left a comment
There was a problem hiding this comment.
LGTM sans remaining two suggestions.
c5d9afc to
3f74736
Compare
|
@bogdandrutu, @jmacd, @reyang can you take another look? You're previous approvals were reset. |
aknuds1
left a comment
There was a problem hiding this comment.
I have doubts regarding this proposed spec change. Please see comment.
| contains all metrics with that `job` and `instance`. `job` and `instance` labels | ||
| MUST be added as resource attributes, and not as metric attributes. |
There was a problem hiding this comment.
Are you sure it's a good idea to add job and instance labels as resource attributes? OTLP -> Prometheus translation considers (according to spec and implementation) instead service.instance.id and service.name to be the identfiying resource attributes. Also, I think that job and instance resource attributes would be dropped in favour of corresponding labels synthesized by Prometheus when generating target_info.
There was a problem hiding this comment.
See this discussion above: #4956 (comment). While our specification stated that service.* attributes were identifying, that was based on a misread of the service attributes specification.
This is now proposing that job and instance are respected if they already exist as resource attributes when ingesting OTLP (since it means that the metric was originally scraped from a prometheus endpoint.
There was a problem hiding this comment.
@dashpole What exactly is based on a misread? That Prometheus' OTLP endpoint treats service.instance.id and service.name resource attributes as identifying? If so, how precisely? Changing it would be backwards incompatible, and I would argue outside of this PR's scope.
What about the second problem I raised, i.e. that job and instance resource attributes would be dropped by the Prometheus OTLP endpoint in favour of corresponding labels synthesized from identifying resource attributes when generating the target_info metric?
This is now proposing that job and instance are respected if they already exist as resource attributes when ingesting OTLP
Where is that stated in the PR's proposed text?
There was a problem hiding this comment.
since it means that the metric was originally scraped from a prometheus endpoint
@dashpole We can't make that assumption. It's but one possible source of job and instance OTel resource attributes.
There was a problem hiding this comment.
We can't make that assumption. It's but one possible source of job and instance OTel resource attributes.
Fair enough. It likely came from a prometheus endpoint, but obviously isn't guaranteed to have. But that isn't really relevant.
What about the second problem I raised, i.e. that job and instance resource attributes would be dropped by the Prometheus OTLP endpoint in favour of corresponding labels synthesized from identifying resource attributes when generating the target_info metric?
I'm not quite sure what you mean. Where in this spec does "labels synthesized from identifying resource attributes when generating the target_info metric" come from? Are you just referring to the fact that we currently derive job/instance from service.name/service.namespace/service.instance.id?
There was a problem hiding this comment.
Where in this spec does "labels synthesized from identifying resource attributes when generating the target_info metric" come from?
@dashpole In addition to how it currently works in Prometheus' OTLP endpoint, it's effectively stipulated in the PR's changes:
If
jobis not present as a resource attribute andservice.nameis present,
theservice.nameandservice.namespaceresource attributes MUST be combined
as<service.namespace>/<service.name>, or<service.name>if namespace is
empty, to form thejobmetric label; otherwise,jobSHOULD be added with an
empty value. Ifinstanceis not present as a resource attribute and
service.instance.idis present,service.instance.idMUST be converted to the
instancelabel; otherwise,instanceSHOULD be added with an empty value.The resulting
jobandinstancelabels MUST be added to all OpenTelemetry
metrics that are associated with the Resource, including thetarget_infometric,
if present.
| If `job` is not present as a resource attribute and `service.name` is present, | ||
| the `service.name` and `service.namespace` resource attributes MUST be combined | ||
| as `<service.namespace>/<service.name>`, or `<service.name>` if namespace is | ||
| empty, to form the `job` metric label; otherwise, `job` SHOULD be added with an | ||
| empty value. If `instance` is not present as a resource attribute and | ||
| `service.instance.id` is present, `service.instance.id` MUST be converted to the | ||
| `instance` label; otherwise, `instance` SHOULD be added with an empty value. |
There was a problem hiding this comment.
On re-reading the PR, I think this must be where you intend to define that job and instance resource attributes respectively should be identifying iff present, with service.name and service.instance.id as respective fallbacks. That should be clearly spelled out, as the intent is effectively ambiguous as currently written.
There are some issues with such a radical change however:
- Users whose resource attributes include
joband/orinstance(doesn't sound entirely unlikely) will experience that their series identities change, when said attributes differ from (service.namespace/ +)service.name/service.instance.id- or when the latter are absent. - The effect of the
keep_identifying_resource_attributesconfig parameter becomes data-dependent: It can only meaningfully apply to theservice.nameandservice.instance.idresource attributes synthesized intojobandinstance. Whenjoband/orinstanceresource attributes take precedence, the former presumably have to be kept ontarget_inforegardless of the parameter.
There was a problem hiding this comment.
I thought about this some more in the meantime - I think that projecting the Prometheus meaning of job and instance onto OTel resource attributes is a flawed design. They don't have this intrinsic meaning within the OTel scope, so I do think that the previous proposal within this PR to prefix them with "prometheus." was an objectively better choice. See also my comment above regarding the assumption that job and instance OTel resource attributes would be proof of Prometheus provenance.
There was a problem hiding this comment.
We will have to deal with how to make this change in prometheus, but I don't think it will be that difficult. It likely just means it will be opt-in until the next prometheus major release.
I don't quite see how adding a prefix changes whether job/instance have meaning. We copy all other labels from prometheus metrics to otel resource/metric labels without prefixes. It isn't clear to me why these labels in particular need prefixes, but the rest of them don't.
Again, i'm open to having it prefixed, but I don't see why it would be objectively wrong to copy label keys between Prometheus and OpenTelemetry as a general rule.
There was a problem hiding this comment.
@dashpole I think there's a misunderstanding around the direction here - AFAICT you are referring to Prometheus -> OTLP conversion, but what I am referring to here is the opposite direction: OTLP -> Prometheus. The specific problem that @krajorama and I are raising is that this proposal would stipulate that the Prometheus OTLP endpoint treat instance and job OTel resource attributes as identifying. We think this is a flawed design, and as a result a problem that will remain even after backwards compatibility itself is no longer required through Prometheus v4.
@krajorama and I would still like have a design doc, so the targeted use cases can be sufficiently clarified. Alternatively we could discuss the proposal during a Prometheus WG call, and produce a doc from that.
krajorama
left a comment
There was a problem hiding this comment.
I agree with @aknuds1 that this is a breaking change for some people.
On Prometheus->OTLP side new labels showing up can in general lead to different results if the query language allows without construct, that is generating series by ignoring some attributes, but allowing everything else.
On OTLP->Prometheus side: user might have knowingly or unknowingly masked job, instance resource attributes by overwriting from service.name, service.id. That would break now as we'd keep job and instance.
Let's discuss at the next WG meeting. See also #4753 (comment)
|
The (resolved) comment thread #4956 (comment) and the comments above lead me to agree that this will be a breaking change, likely in more than one way. See particularly the comment by @ArthurSens #4956 (comment): we're going to need Collector documentation updates for the users that have become used to the service.* resource attributes. |
| ([#4605](https://github.com/open-telemetry/opentelemetry-specification/issues/4605)) | ||
| - Add optional OpenTelemetry Histogram to Prometheus Native Histogram with Custom Buckets transformation. | ||
| ([#4605](https://github.com/open-telemetry/opentelemetry-specification/issues/4605)) | ||
| - Prometheus `job` and `instance` labels are preserved as resource attributes when converting to OTLP. |
There was a problem hiding this comment.
For my own future reference, so I don't forget; this changelog entry doesn't mention that the PR changes the other way round too: From OTLP to Prometheus.
|
The backward compatibility concerns are real, but I think they're worth it — this PR addresses two pain points that come up a lot in practice:
Overall a solid step forward — happy to see this getting spec-level attention. |
How is this solved by respecting |
|
I want to read the phrasing "If |
The problem is that it doesn't only say that. In practice it says to respect |
|
Point taken. To keep momentum, I'd suggest prioritizing the Prometheus → OTLP direction and specifically making the derivation of |
aknuds1
left a comment
There was a problem hiding this comment.
This should be held off on until the design doc reaches consensus.
|
We scrape KSM with the prometheus receiver and then re-associate each series with the workload it actually describes (the pod, deployment, etc.) using k8s_attributes. The trouble is that the Prometheus receiver puts Chiming in because this pattern is common enough that a first-class opt-out would save everyone from writing the same workaround to manually drop these attributes |
#16285) ## Summary Adds a per-tenant counter `cortex_distributor_otlp_requests_with_job_or_instance_resource_attribute_total{user}` in the distributor. It increments once for each OTLP request whose payload contains at least one `ResourceMetrics` carrying `job` or `instance` as a raw resource attribute key. The detection is folded into the existing ResourceMetrics pass (`inspectOTLPResourceMetrics`, renamed from `observeOTLPFieldsCount`) and short-circuits after the first match, so the overhead is two `pcommon.Map` lookups per resource until a hit — negligible.⚠️ This is **experimental** and expected to be short-lived. It exists to gather production data for the OpenTelemetry specification PR open-telemetry/opentelemetry-specification#4956, which is deciding how OTel senders should convey `job` and `instance` identity. Once we have enough signal on how prevalent this pattern is across tenants, the metric will most likely be removed. Because it's short-lived and diagnostic in nature, expect it to disappear in a future release; please do not build long-lived dashboards or alerts on it. ## Test plan - [x] `go test ./pkg/distributor/ -run TestInspectOTLPResourceMetrics` — new table-driven test covering no-match, job-only, instance-only, both, and match-on-later-resource cases - [x] `go test ./pkg/distributor/ -run TestHandlerOTLPPush` — existing OTLP handler tests still pass - [x] `go build ./pkg/distributor/... && go vet ./pkg/distributor/...` 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull request dashboard statusWaiting on the author · refreshed 2026-08-11 20:13 UTC Move out of draft to request review. Status above doesn't look right?
|
Part of #4753
Fixes #4577
Related to open-telemetry/opentelemetry-collector-contrib#45982
Issues with the current state
Prometheus scrape identity (
job/instance) and OpenTelemetry semantic identity (service.name/service.instance.id) are similar, but ultimately aren't identical concepts, which manifests in a few ways. Currently, when scraping a target that exposestarget_infowithservice.*labels, receivers are forced to drop eitherjob/instanceorservice.name/service.instance.id. Additionally, users pushing OTLP to Prometheus expect to query forservice.name/service.instance.idlike any other resource label, but today must configure server-side flags (keep_identifying_resource_attributes=true) to retain them.Changes
jobandinstancelabels are preserved as distinct OTel Resource Attributes alongsideservice.name/service.instance.id.service.nameandservice.instance.idMAY default tojobandinstancewhen absent ontarget_info. Implementations MUST provide a configuration option to disable this defaulting behavior.jobandinstanceresource attributes first, falling back to<service.namespace>/<service.name>andservice.instance.idif absent.Alternatives Considered
prometheus.job/prometheus.instance:jobandinstanceare preferred for consistent treatment with other Prometheus labels and for a more intuitive UX in Collector processors / OTTL.service.*fromjob/instanceby default: Rejected as the default because it would be a breaking change for existing OTel Collector Prometheus receiver users, and the only benefit is less duplication.@open-telemetry/prometheus-interoperability @aknuds1 @kln21002 @cyrille-leclerc