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

Initial Prometheus <-> OTLP datamodel specification. #2266

Merged
merged 27 commits into from
Feb 18, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Jan 14, 2022

This supersedes #2017, and is meant to capture the current state of prometheus support in prometheus SDK exporters and prometheus collector receivers/exporters, rather than the desired end state. This is done to avoid (potentially) contentious debates in the initial draft, and allow tackling these topics individually.

There are a number of changes we will likely want to make after this:

  • Handling currently unsupported types (StateSet, Info, GaugeHistogram, ExponentialHistogram)
  • Converting OTel resources to something in prometheus

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
Copy link
Member

@alolita alolita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Prometheus should be consistently capitalized.

@dashpole
Copy link
Contributor Author

Nit: Prometheus should be consistently capitalized.

Done

@dashpole dashpole marked this pull request as ready for review January 19, 2022 22:55
@dashpole dashpole requested review from a team January 19, 2022 22:55
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

@dashpole not sure if prometheus let users set their "job" in the metrics source, but if that is the case probably the library sdk prom exporter should convert "service.name" to "job". Also a separate PR is fine.

@dashpole
Copy link
Contributor Author

not sure if prometheus let users set their "job" in the metrics source, but if that is the case probably the library sdk prom exporter should convert "service.name" to "job".

It is possible to set "job" in the metrics source, but by default it will be overwritten by service discovery. honor_labels in the prom scrape config allows you to override this, but it is generally intended for use with a federated prom endpoint, rather than an application endpoint. I'll plan to address this when addressing #1782 in a follow-up.

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conversions statements here are not made using the normative language conventions this specification is defined to use. Can we update these statements to make clear requirements and recommendations about these conversions?

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor Author

dashpole commented Feb 3, 2022

Can we update these statements to make clear requirements and recommendations about these conversions?

Done.

@carlosalberto
Copy link
Contributor

This looks like it's ready to go. Please confirm there's no blocker at this moment @brian-brazil @legendecas @anuraaga @jmacd

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍nice works!

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu merged commit b35f18f into open-telemetry:main Feb 18, 2022
@dashpole dashpole deleted the current-prom-spec branch February 18, 2022 18:02
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
…2266)

* Initial draft of Prometheus <-> OTLP datamodel specification.

* Update specification/metrics/datamodel.md

Co-authored-by: Joshua MacDonald <[email protected]>

* Add blurb about labels and incompatibilities.

* address minor review comments, and fix presubmit failures

* update prometheus SD labels based on current state, and update formatting

* handle start time and delta sums

* add links and headers.  Handle exemplars and otel resource attributes.

* Apply suggestions from code review

Co-authored-by: Anthony Mirabella <[email protected]>

* fix link

* change prom service discovery section to resource attribute sections

* disallow process_start_time_seconds

* add links for otel data points

* Apply suggestions from code review

Co-authored-by: Tyler Yahn <[email protected]>

* use normative language

* handle negative prometheus histogram buckets

* typo

* specify what to do with non-string attributes

* clarify sum behavior

* handle label collisions when converting

* clarify that if deltas are not aggregated, they must be dropped

* allow process_start_time_seconds, but specify that it must be disabled by default

* Update specification/metrics/datamodel.md

Co-authored-by: Tyler Yahn <[email protected]>

* Update specification/metrics/datamodel.md

Co-authored-by: legendecas <[email protected]>

* Apply suggestions from code review

Co-authored-by: Joshua MacDonald <[email protected]>

* describe when process_start_time_seconds isn't correct

* convert values to string

Co-authored-by: Josh Suereth <[email protected]>
Co-authored-by: Josh Suereth <[email protected]>
Co-authored-by: Joshua MacDonald <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Anuraag Agrawal <[email protected]>
Co-authored-by: legendecas <[email protected]>
Co-authored-by: Bogdan Drutu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.