[receiver/prometheus] Histograms without buckets are dropped

[swiatekm]

Component(s)

receiver/prometheus

What happened?

I wanted to ingest just the sum and count timeseries for a histogram using prometheus receiver, and used an appropriate relabel config. But the receiver dropped the histogram.

Description

If we ingest a histogram without buckets - only the count and sum parts - prometheusreceiver drops it.

Steps to Reproduce

Simplest way is probably to modify data for one of the unit tests. I verified it with

opentelemetry-collector-contrib/receiver/prometheusreceiver/metrics_receiver_test.go

Line 1331 in 19cacb0

func TestStartTimeMetric(t *testing.T) {

.

Expected Result

I'd have expected to get a histogram without buckets. The spec says this is valid.

Actual Result

The histogram was dropped.

Collector version

0.77.0

Additional context

What I'm trying to do is to ingest only the sum and count parts of the histogram. This works on Prometheus itself.

The alternative would be to ingest the buckets, and then drop them in a processor. There currently aren't any functions to do this to histograms, but we do have ones for summaries, so it wouldn't be a stretch to add them.

I believe this happens as a result of the following check:

opentelemetry-collector-contrib/receiver/prometheusreceiver/internal/metricfamily.go

Line 90 in 19cacb0

if !mg.hasCount || len(mg.complexValue) == 0 {

.

I haven't verified if this affects simpleprometheusreceiver, but I assume it does.

[github-actions]

Pinging code owners:

• receiver/prometheus: @Aneurysm9 @dashpole

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[dashpole]

Requiring prometheus histograms to have at least one bucket is in-line with the OpenMetrics spec for histograms:

A Histogram MetricPoint MUST contain at least one bucket, and SHOULD contain Sum, and Created values. Every bucket MUST have a threshold and a value. ... Histogram MetricPoints MUST have one bucket with an +Inf threshold.

So it MUST have an Inf bucket, whose value is the same as the sum of the series.

An OpenTelemetry histogram without any buckets MUST, per the spec, have an Inf bucket if it is exported by a Prometheus exporter:

A series of {name}_bucket metric points that contain all attributes of the histogram point recorded as labels. Additionally, a label, denoted as le is added denoting the bucket boundary. The label’s value is the stringified floating point value of bucket boundaries, ordered from lowest to highest. The value of each point is the sum of the count of all histogram buckets up the the boundary reported in the le label. These points will include a single exemplar that falls within le label and no other le labelled point. The final bucket metric MUST have an +Inf threshold.

How did you encounter a prometheus histogram without any buckets?

[swiatekm]

How did you encounter a prometheus histogram without any buckets?

I dropped the buckets at ingest using a relabeling rule. To give a more specific example, the Kubernetes Apiserver exposes a apiserver_request_duration_seconds histogram metric. This metric has fairly high cardinality, and a lot of the time you can make do with just the sums and counts, so we'd dropped the buckets using roughly the following:

- action: keep
  regex: apiserver_request_duration_seconds_(?:count|sum)
  sourceLabels:
  - __name__

If I do this using plain Prometheus, I will get apiserver_request_duration_seconds_count and apiserver_request_duration_seconds_total as Counters. I am fine if the receiver did this as well, but given that Otel allows empty histograms, I thought this would be a simpler solution.

[swiatekm]

There's also #22853 as a solution to the same problem, but it's not as efficient to have to scrape and process the whole Histogram just to drop all the buckets later.

[swiatekm]

@dashpole in a similar vein, the receiver rejects a Summary without quantiles, while Prometheus happily accepts it.

I'm reporting this in the context of attempting to migrate a large K8s environment from Prometheus to Otel with prometheus receiver. And so I need to deal with targets producing metrics that aren't up to spec, with scrape configs I don't own or control, and other fun bits like that.

I suppose the fundamental problem here is that Prometheus itself is very permissive about what it accepts, and doesn't actually check any of the spec requirements. Whereas the receiver is more strict, both on the OpenMetrics and Otel sides.

At a high level, what can we do about this? Some kind of permissive mode where if we run into a problem, we just shrug and ingest the datapoint as Unknown? The Histogram problem which this issue describes can be worked around later in the Otel pipeline, but the Summary problem causes the whole scrape to fail, so the only solution seems to be to fix all the consumers. It would at the very least be nice if we could ingest all the correct data from a given scrape even if there are errors along the way.

[dashpole]

Thanks, and i'm sorry for making it harder on you. I think we should revisit some of the validation, especially for summaries. Off the top of my head, I don't remember why we require quantiles. The only thing that would make sense would be if quantiles were required in OTLP... if they aren't, we should definitely remove that requirement. OTel only has summaries to support these sort of "legacy" cases, so extra validation doesn't make any sense for summaries.

My recollection of the +Inf bucket requirement for histograms was that it protected against client-side bugs where some histogram buckets were missing from the exposition. It might be worth making it configurable, or dropping the requirement all-together.

[dashpole]

I'll raise this at the prometheus wg next wednesday

[dashpole]

Summary quantiles should be optional: https://github.com/open-telemetry/opentelemetry-proto/blob/c4dfbc51f3cd4089778555a2ac5d9bc093ed2956/opentelemetry/proto/metrics/v1/metrics.proto#L634. We should definitely remove the requirement to have a quantile.

For histograms, the +Inf bucket is always the same as the _sum series. We can either:

1. Always ignore the +Inf bucket
2. Validate that the +Inf bucket exists, and/or validate that it is the same as the sum.

[swiatekm]

Summary quantiles should be optional: https://github.com/open-telemetry/opentelemetry-proto/blob/c4dfbc51f3cd4089778555a2ac5d9bc093ed2956/opentelemetry/proto/metrics/v1/metrics.proto#L634. We should definitely remove the requirement to have a quantile.

Good to hear. I'll create a new issue for this, and can work on the change afterwards, if that's ok.

For histograms, the +Inf bucket is always the same as the _sum series. We can either:

1. Always ignore the +Inf bucket

2. Validate that the +Inf bucket exists, and/or validate that it is the same as the sum.

Maybe just validate that the value of this bucket is correct, if it exists? If it doesn't exist, we can just ignore it.

[dashpole]

I was the only one at the wg meeting today, so I wasn't able to discuss it. @Aneurysm9, unless you have concerns, I think we should start to ignore the +Inf bucket. The only potential negative consequence would be that the +Inf bucket would "reappear" if you exported it using a prometheus exporter, but that seems unlikely to cause issues.

[swiatekm]

@dashpole is there a reason not to merge #23448? That change is relatively benign and non-breaking, whereas dropping +Inf buckets is technically breaking, so it'd be better to do it separately imo.

[dashpole]

Ignoring the Inf bucket shouldn't be breaking, right? It is always the same as the count.

[swiatekm]

I think I was misunderstanding what you wanted to do there. So, just to be clear:

• The current state is that we calculate our final OTLP bucket (the one without a right bound) from the +Inf Prometheus bucket.
• We want the output to be identical, but instead calculate it from the total count, and just ignore the value of the +Inf Prometheus bucket completely.

Is that right?

[dashpole]

Yes. I think the end result would be something that produces a single OTel bucket even if there are no prometheus buckets.

[swiatekm]

Allright, that's what I've done in #23448.

[swiatekm]

Fixed in #23448