Skip to content
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

Clarify timestamps should not be added in prometheus export and remove current references #2468

Closed
anuraaga opened this issue Apr 4, 2022 · 5 comments · Fixed by #3872
Closed
Assignees
Labels
spec:metrics Related to the specification/metrics directory

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Apr 4, 2022

OpenMetrics spec gives explicit advice to not include timestamps in exported data.

https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#exposing-timestamps

So we should probably follow this. Currently, we only mention timestamps for exemplars

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#exemplars-2

We should remove this and add explicit text that timestamps MUST NOT be included to match the OpenMetrics guidance.

Note that timestamps can cause problems in the wild presumably due to Prometheus implementation limitations even when the format seems valid from an OpenMetrics standpoint.

open-telemetry/opentelemetry-java#4193

@anuraaga
Copy link
Contributor Author

@open-telemetry/wg-prometheus Do you have any thoughts on this? Thanks

@dashpole
Copy link
Contributor

There are use-cases in prometheus (e.g. collecting metrics from another metrics system), where timestamps should be used. IIRC, those aren't possible in with the current otel API (you can't specify that an observation occurred at some point in the past), so it makes sense to specify that SDK exporters MUST NOT expose timestamps. If otel gains the ability to record observations at a time in the past, we would want to expose an option to include timestamps. But for now, MUST NOT include timestamps in SDK exporters makes sense.

For the collector's prometheus exporter, it makes sense to include timestamps, since metrics may have been collected at a point further in the past. Including timestamps matches the behavior of the prometheus server's federation endpoint (I just checked).

@jsuereth
Copy link
Contributor

jsuereth commented Feb 8, 2024

I'm not sure I agree on exemplars unless prometheus doesn't support their timestamps correctly.

UNLIKE prometheus metrics where everything is cumulative, exemplars are snapshots of when something was recorded and the timestamp is as important as the value, particularly in cumulative instance where we may be carrying around stale exemplars.

@dashpole
Copy link
Contributor

dashpole commented Feb 8, 2024

@anuraaga I think I understand your confusion now. The OpenMetrics guidance to not expose timestamps is only meant to apply to metric timestamps, not exemplar timestamps. We can, and should use timestamps with prometheus exemplars, but should not use timestamps for data points.

@dashpole
Copy link
Contributor

dashpole commented Feb 8, 2024

From the OM spec:

in general, MetricPoint timestamps should not be exposed

The section is only talking about timestamps on MetricPoints, not on exemplars

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
Fixes
open-telemetry#3702
Fixes
open-telemetry#2468

Addresses
open-telemetry#3737 (comment)

## Changes

* Add option to disable `target` info metric.
* Move SDK-specific language from the compatibility spec to the
prometheus exporter spec.
* Disallow using OpenMetrics proto format, or Prometheus remote write
formats in SDK exporters
* Recommend using Prometheus client libraries
* Clarify that only features supported by the text format are required
to be implemented by Prometheus exporters
* Require prometheus SDK exporters to follow the compatibility
specification.

@open-telemetry/wg-prometheus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants