Skip to content

Conversation

@laurit
Copy link
Contributor

@laurit laurit commented Nov 4, 2024

@trask WDYT?

@github-actions github-actions bot requested a review from theletterf November 4, 2024 12:38
@trask
Copy link
Member

trask commented Nov 4, 2024

I'm not sure we can do this, it looks like exposition format is controlled by the client scraper via the Accept http header:

https://github.com/prometheus/client_java/blob/fc583c2e5813d4ab510f3464f7bf5d2f9b4d3ce2/prometheus-metrics-exposition-formats/src/main/java/io/prometheus/metrics/expositionformats/ExpositionFormats.java#L34-L36

@laurit
Copy link
Contributor Author

laurit commented Nov 5, 2024

I'm not sure we can do this, it looks like exposition format is controlled by the client scraper via the Accept http header

Thanks. https://opentelemetry.io/docs/specs/otel/metrics/sdk_exporters/prometheus/ doesn't say that the protobuf format must be supported. As far as I can tell support for the protobuf format was added not too long ago with open-telemetry/opentelemetry-java#6015

@trask
Copy link
Member

trask commented Nov 5, 2024

added not too long ago with open-telemetry/opentelemetry-java#6015

this part is interesting:

In Prometheus, exponential histograms require the Prometheus protobuf exposition format.

@laurit
Copy link
Contributor Author

laurit commented Nov 5, 2024

Prometheus seems to support OTLP https://prometheus.io/docs/guides/opentelemetry/

@trask
Copy link
Member

trask commented Nov 6, 2024

@fstab any thoughts?

@fstab
Copy link
Member

fstab commented Nov 6, 2024

In Prometheus, exponential histograms require the Prometheus protobuf exposition format.

Yes, exponential histograms are only supported in Prometheus protobuf format, not in Prometheus text format. There's work underway to specify a text representation, but that's not implemented yet.

If a Prometheus scraper runs with exponential histogram support enabled, it will signal protobuf support in the HTTP Accept: header, and then the Prometheus exporter will expose protobuf format, including exponential histograms.

I don't think this should be removed, otherwise support for exponential histograms would be lost.

@fstab
Copy link
Member

fstab commented Nov 6, 2024

Also, as the protobuf dependency is shaded into the io.prometheus package there should not be a conflict with the protobuf version used by the OTel SDK itself.

@laurit
Copy link
Contributor Author

laurit commented Nov 6, 2024

I don't think this should be removed, otherwise support for exponential histograms would be lost.

I created this PR under the assumption that text format can represent everything. I'll close it as we don't wish to loose this functionality.

Also, as the protobuf dependency is shaded into the io.prometheus package there should not be a conflict with the protobuf version used by the OTel SDK itself.

Neither the otel sdk nor the agent depend on the protobuf lbrary. The size of protobuf library is bit less than 2mb which is roughly 9% of the agent size.

@laurit laurit closed this Nov 6, 2024
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.

3 participants