-
Notifications
You must be signed in to change notification settings - Fork 916
[Prometheus and OpenMetrics] Handle scheme URL and scope attributes as identifying #4223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@dashpole, are you able to take this? |
cc @open-telemetry/wg-prometheus Yes, I'll take this |
otel_scheme_url
label to otel_scope_info
metric
otel_scheme_url
label to otel_scope_info
metricotel_scheme_url
label to otel_scope_info
metric
otel_scheme_url
label to otel_scope_info
metric
The idea is to add I will do my best to help with this issue. |
A prototype in Go: open-telemetry/opentelemetry-go#5947 |
Based on the prototype, I think this could be split into 4 sub-issues:
|
This seems like the right path going forward. Now that these are all identifying, we CANNOT rely on joins for :EDIT: or if we did, we'd need some kind of "unique id" to perform a join based on all identifying attributes. |
@open-telemetry/prometheus-interoperability, @open-telemetry/collector-maintainers: PTAL |
On the Collector side, I think there are no issues as long as the scope attribute set is 1. exposed to the user, and 2. identifying on the Prometheus side, to avoid errors with time series collisions. Adding the attributes as (Unrelated, but what is the distinction between sub-issues 1 and 2 above?) |
I have forgotten to delete "and scope attributes"` from sub-issue 1 😅 Fixed 😉 I will do my best to move this issue forward. However, I will start in about 2 weeks. In the meantime you can try prototype it on the collector side and to test everything e2e together with open-telemetry/opentelemetry-go#5947 to make sure that there are not issues with this proposal. Having such e2e prototypes (Collector + Go) will help move the specification work forward. |
I made a corresponding Prometheus issue. |
@pellared By "prototyping on the Collector side", did you mean in the Prometheus exporter in collector-contrib? Based on the discussion in today's Collector SIG meeting, it sounds like this part in CONTRIBUTING.md would make this not "count" as a prototype for the purpose of changing the spec:
In other words, we may need a prototype in another language SDK's Prometheus exporter |
Filed #4497 to reword the prototype requirement |
@jade-guiton-dd, I think that Prometheus receiver in collector-contrib is also crucial (not only exporter). |
Yes of course, everything performing translation between OTel and Prometheus will need to be updated eventually. I was just wondering about what prototypes you think need to be done to get the spec change moving. Re-reading your message, I think I misunderstood. By "e2e prototypes (Collector + Go)", did you mean a single prototype where the draft PR modifying the SDK exporter is integrated into the Collector to test that it fixes our issue when producing scope attributes? |
No. I just mean building 3 prototypes: Go SDK Prometheus exporter, Collector Prometheus receiver, Collector Prometheus exporter and check if they will work together correctly (even "manually"). I just think it would be nice to check all components starting from an instrumented application, through Collector, finishing in Prometheus. I am fine with any other validation/testing approach. |
I updated the OTel Go's exporter implementation here: open-telemetry/opentelemetry-go#5947. PTAL |
PoC for Prometheus exporter in collector-contrib: open-telemetry/opentelemetry-collector-contrib#40004 Reviews appreciated, especially in test coverage of edge cases :) |
@ArthurSens, are you going to also work on prototype for prometheusreceiver? |
Given open-telemetry/opentelemetry-collector-contrib#40004 (comment):
It looks like we can drop point 3 from #4223 (comment) completely ( |
I spent some time yesterday and it was not as easy as I expected, but I'm also not 100% familiar with that codebase 😬 I plan to spend more time on it this week, but it might take a while to have a PoC |
I created a specification PR. PTAL: |
…ls (#5947) Fixes #5846 Towards open-telemetry/opentelemetry-specification#4223 (first two points from open-telemetry/opentelemetry-specification#4223 (comment)) - Add instrumentation scope schema URL as `otel_scope_schema_url` label to exported metrics - Add instrumentation scope attributes as `otel_scope_[attribute]` labels to exported metrics Side notes: - The exporter does not seem to work correctly if the end user adds e.g. an `otel_scope_name` attribute. I think the exporter should ignore all attributes with names that have prefix `otel_scope_`. This scenario is also not covered by the spec. I think it may be covered by a separate PR and issue. - Removal of `otel_scope_info` metric would be handled as a separate PR. #6770
Fixes #6768 Towards open-telemetry/opentelemetry-specification#4223 (last point from from open-telemetry/opentelemetry-specification#4223 (comment)) - Remove instrumentation scope attributes as `otel_scope_info` metric
…butes as labels (#40004) #### 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_info` with 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. --------- Signed-off-by: Arthur Silva Sens <[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]>
Uh oh!
There was an error while loading. Please reload this page.
I think this needs updates to some of the compatibility docs (at least prometheus). Currently, Prometheus adds scope name and scope version as labels to ensure we don't end up with duplicate timeseries, and we are able to put scope attributes in a separate
otel_scope_info
metric. With this change, we will need to update this to instead add the scope name, scope version, and scope attributes to all metrics.TIL
schema_url
is also identifying, so that is something we will need to handle in Prometheus export and other "non-OTLP" compatibility documents.Originally posted by @dashpole in #4161 (comment)
The text was updated successfully, but these errors were encountered: