Repository navigation
Conversation
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Pull request dashboard statusWaiting on the author · refreshed 2026-11-09 08:18 UTC Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):
Status above doesn't look right?
|
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
ArthurSens
left a comment
There was a problem hiding this comment.
initial pass on the proposed changes :)
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
| | `server.port` | The `<port>` portion of the target's URL that was scraped | | ||
| | `url.scheme` | `http` or `https` | | ||
| | `server.address` | The `<host>` portion of the `instance` unless it is localhost, a loopback address, or an unspecified address | | ||
| | `server.port` | The `<port>` portion of the `instance` | |
There was a problem hiding this comment.
I think we should keep server.address and server.port defined in terms of the scraped target address rather than instance (and omit server.port when no port is present, with an integer value per semconv), and treat the receiver's instance parsing as a bug.
There was a problem hiding this comment.
Since the existing spec on main already defines server.address and server.port in terms of the target's URL that was scraped, could we keep the target's URL that was scraped here (rather than changing it to instance) and file an issue in opentelemetry-collector-contrib to fix prometheusreceiver?
There was a problem hiding this comment.
sounds like a plan, let's discuss in the meeting and I can make the change back or whatever we decide to write here
There was a problem hiding this comment.
in the meeting: we don't want to do this in the remote write receiver, prefer to set at scrape time only as original data
There was a problem hiding this comment.
I also found from my review that we should preserve server.address and server.port in terms of the scraped target address.
There was a problem hiding this comment.
dashpole opened open-telemetry/opentelemetry-collector-contrib#52570
There was a problem hiding this comment.
Now that open-telemetry/opentelemetry-collector-contrib#52570 is merged, we can update server.address, server.port, and url.scheme here to reference the scraped target's URL (or post-relabeling __address__ and __scheme__ labels) rather than instance and pre-relabeling __scheme__.
Restores the #metric-attributes anchor for the OTLP to Prometheus section, which is referenced from within the document and from the spec compliance matrix. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
…52407) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Noticed that this was wrong while working on open-telemetry/opentelemetry-specification#5356 and looking into how `server.address` is calculated. Has been wrong since #8266 switched the key to `net.host.name` and later it became `service.address` (#40572). <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation <!--Authorship attestation. See AGENTS.md for details. AI agents must not check this box on behalf of the user; the human author must check it themselves before the PR is ready for review.--> #### Authorship - [x] I, a human, wrote this pull request description myself. <!--Please delete paragraphs that you did not use before submitting.--> Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
| | `server.port` | The `<port>` portion of the target's URL that was scraped | | ||
| | `url.scheme` | `http` or `https` | | ||
| | `server.address` | The `<host>` portion of the `instance` unless it is localhost, a loopback address, or an unspecified address | | ||
| | `server.port` | The `<port>` portion of the `instance` | |
There was a problem hiding this comment.
I also found from my review that we should preserve server.address and server.port in terms of the scraped target address.
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
Move the target info rules to the common part of the Resource Attributes section and add separate subsections for scraped metrics and metrics received over Prometheus Remote Write. The push rules describe how service.name, service.namespace and service.instance.id are derived from the job and instance labels, matching the Prometheus Remote Write receiver behavior. Clarify that url.scheme is taken from the scrape target before relabeling. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
| For each resource the `service.name` attribute MUST be set to the | ||
| `service.name` label in `target` info metric, if that label exists in the | ||
| `target` info metric with the same `job` and `instance` key as the resource. | ||
| Otherwise `service.name` MUST be set to the value of the `job` label. |
There was a problem hiding this comment.
The prometheusremotewritereceiver implementation currently splits job by / to derive service.namespace and service.name. If we agree this shouldn't happen, we should treat it as an implementation bug to be fixed rather than codifying the split here in the spec. I have opened open-telemetry/opentelemetry-collector-contrib#52626 to fix this.
|
Hi @krajorama — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.
|
…els from datapoint attributes (#52736) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Current behavior is to drop `__scheme__` and `__metrics_path__` from metric labels when creating the series identity and data point attributes. However these labels are usually scrape target labels that are never actually added to series labels by Prometheus itself. The only way these actually appear as series labels if the user directly exposed them or added them by metric relabel rules. Since Prometheus itself doesn't prohibit this (only advises against) , dropping them is against the stated goal of the receiver to be a drop in Prometheus replacement. It's a good question if this is a breaking change or simple bugfix. I've tried to trace the history of this using LLM, see Appendix at the bottom. <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue Related to open-telemetry/opentelemetry-specification#5356 <!--Describe what testing was performed and which tests were added.--> #### Testing Unit tests. <!--Describe the documentation added.--> #### Documentation TBD - let's decide if we go forward with this. <!--Authorship attestation. See AGENTS.md for details. AI agents must not check this box on behalf of the user; the human author must check it themselves before the PR is ready for review.--> #### Authorship - [x] I, a human, wrote this pull request description myself. <!--Please delete paragraphs that you did not use before submitting.--> #### Appendix A TL;DR; excluding `__scheme__` / `__metrics_path__` didn't make sense in OpenCensus either, they were never put on series labels, they were used to create the `Node`. History research with LLM: I [LLM] found no written explanation. The `__scheme__` / `__metrics_path__` exclusion was there from the receiver's first version in OpenCensus in 2019. Even then, Prometheus didn't put __ target labels on samples, so the entry looks like a defensive copy of the target's identity labels rather than a fix for something actually seen. Where it came from: census-instrumentation/opencensus-service#572 PR #572 mentions the metrics path label only in code, never in prose. I searched the full diff, the description, review bodies, inline review comments and conversation comments for metrics_path, MetricsPath, "metrics path", scheme and useful. `__metrics_path__` (model.MetricsPathLabel) appears in two places: `isUsefulLabel` in the new `receiver/prometheusreceiver/internal/metricsbuilder.go` (lines 402-408 at the PR's first commit 76015a5d7). It has no comment. It's an empty case with the other names, so they all fall through to result = false: ```go func isUsefulLabel(mType metricspb.MetricDescriptor_Type, labelKey string) bool { result := false switch labelKey { case model.MetricNameLabel: case model.InstanceLabel: case model.SchemeLabel: case model.MetricsPathLabel: case model.JobLabel: ... ``` It's called from `updateLabelKeys`, which decides which sample labels become OpenCensus label keys. `Test_isUsefulLabel` in `metricsbuilder_test.go`: one case,` {"metricPath", args{GAUGE_DOUBLE, model.MetricsPathLabel}, false}`, which only checks that the list says what it says. `__scheme__` shows up in a few more places, and these do explain part of the grouping: `transaction.go` `initTransaction` builds the node with `createNode(job, instance, mc.SharedLabels().Get(model.SchemeLabel))`. In `metadata.go`, `mCache.SharedLabels()` returns `target.DiscoveredLabels()`. So the scheme is read from the target's discovered labels, not from sample labels, the same way `orijtech/promreceiver` did it. The test mock in `internal_test.go` has `SharedLabels()` return `labels.FromStrings("__scheme__", "http")`. So the PR uses `__scheme__` as a target-level value for the node and also lists it, with `__metrics_path__`, `job` and `instance`, as "not useful" sample labels. That supports my guess that the author excluded everything that identifies the target, though no one wrote it down. For `__metrics_path__` specifically, there's no stated reason, and the PR never reads it anywhere else. In [census-instrumentation/opencensus-service#572](census-instrumentation/opencensus-service#572), `Node` is OpenCensus's `commonpb.Node` from the OpenCensus agent protocol (`opencensus-proto`, `agent/common/v1/common.proto`). It describes the source the data came from: a process identifier (host name, pid, start time), library info, service info and free-form string attributes. OpenCensus exports sent a `Node` and a separate `Resource` next to the metrics. OpenTelemetry folded both into the single Resource. Collector core added the OpenCensus-to-internal translation in 2020 ([open-telemetry/opentelemetry-collector#596](open-telemetry/opentelemetry-collector#596), [open-telemetry/opentelemetry-collector#763](open-telemetry/opentelemetry-collector#763)), which turns Node fields into resource attributes. The receiver's current [`CreateResource`](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/970f597792f2e60786e480dad4598b1078ba5627/receiver/prometheusreceiver/internal/prom_to_otlp.go#L35-L53) is the direct successor of the PR's `createNode`: | `createNode` in #572 (2019) | `CreateResource` today | |---|---| | `ServiceInfo.Name = job` | `service.name = job` | | `Identifier.HostName` = host part of `instance` | `server.address` = host, only if it's a real host name rather than an IP or `localhost` | | `Attributes["port"]` = port part of `instance`, default `"80"` | `server.port` | | `Attributes["scheme"]` = `__scheme__` from the target's discovered labels | `url.scheme`, from the same source | | (nothing) | `service.instance.id = instance` | | (nothing) | Kubernetes attributes from `__meta_kubernetes_*` discovered labels | So the resource attributes all come from `job`, `instance` and the target's discovered labels, never from sample labels. That matches the exclusion list: - `job` and `instance` are dropped from data points because they're on the resource. - `__scheme__` is on the resource too, but it comes from discovered labels. Dropping it from samples only matters in the two unusual cases: the target exposes it, or a relabel rule creates it. - `__metrics_path__` was never part of the Node or the Resource, so its spot on the list doesn't even match that reasoning. --------- Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
| When receiving metrics over Prometheus Remote Write, a missing `job` or | ||
| `instance` label MUST be treated as an empty strings when creating a new | ||
| resource for the metrics. |
There was a problem hiding this comment.
I don't understand what this means 🤔
What do you mean we'll create a new resource from empty strings?
There was a problem hiding this comment.
I've tried to clarify: 401d5cc7146cfd177933d2ef98a15d271c92f8dc
| For each resource the `service.name` attribute MUST be set to the | ||
| `service.name` label in `target` info metric, if that label exists in the | ||
| `target` info metric with the same `job` and `instance` key as the resource. | ||
| Otherwise, if the `job` is not the empty string, then `service.name` MUST be | ||
| set from the `job` label as follows: |
There was a problem hiding this comment.
Just to be sure, here we're clarifying the already existing behavior, right? And we want to discuss how to change this behavior in the future... is that right?
| - Clarify the conversion of Prometheus labels and the `target` info metric to | ||
| OTLP resources, data point attributes and resource attributes. | ||
| ([#5356](https://github.com/open-telemetry/opentelemetry-specification/pull/5356)) |
There was a problem hiding this comment.
Do we need a changelog if we're only clarifying existing behavior?
There was a problem hiding this comment.
I think it's a significant enough rewrite , yes.
There was a problem hiding this comment.
In fact we're changing something , so I added another item:
6c6e850
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Due to open-telemetry/opentelemetry-collector-contrib#52570 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
This PR originates from discussions around #4956. The proposed changes in that PR and subsequent solution options in the design doc turned out to be hard to formulate in normative language as the baseline isn't specific enough and doesn't match exactly what was actually implemented.
The scope of this PR is:
Out of scope, these shall be put into their own issues/PRs.
Fixes N/A
Changes
Please provide a brief description of the changes here.
For non-trivial changes, follow the change proposal process.
service.name,service.namespace, andservice.instance.idas Prometheus metric labels #4577CHANGELOG.mdfile updated for non-trivial changes[chore]in the PR title to skip the changelog check