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

Prometheus receiver is not enabling native histograms feature #26555

Closed
rishabhkumar92 opened this issue Sep 8, 2023 · 7 comments · Fixed by #28663
Closed

Prometheus receiver is not enabling native histograms feature #26555

rishabhkumar92 opened this issue Sep 8, 2023 · 7 comments · Fixed by #28663
Labels
enhancement New feature or request priority:needed Triagers reviewed the issue but need code owner to set priority receiver/prometheus Prometheus receiver Stale

Comments

@rishabhkumar92
Copy link

rishabhkumar92 commented Sep 8, 2023

Component(s)

receiver/prometheus

What happened?

Description

Prometheus receiver is not enabling native histograms feature. We have a usecase where we are using Prometheus receiver and PRW exporter to export metrics.

Steps to Reproduce

I have verified that Prometheus receiver is not able to scrap native histogram metrics while vanilla prometheus works with enabling native histogram feature.

Expected Result

Native histogram should be able to be scraped.

Actual Result

Not able to see native histogram after verifying from logging exporter.

Collector version

0.80

@rishabhkumar92 rishabhkumar92 added bug Something isn't working needs triage New item requiring triage labels Sep 8, 2023
@github-actions github-actions bot added the receiver/prometheus Prometheus receiver label Sep 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

Pinging code owners:

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

@bryan-aguilar
Copy link
Contributor

I believe it may be best to wait for histogram textual representation to be defined in the prometheus community instead of supporting the protobuf format. I don't think anything has been documented around that decision though.

This is the prometheus issue that was found regarding text format for histogram prometheus/prometheus#11265

cc'ing @gouthamve also on this one.

@dashpole
Copy link
Contributor

We would need to implement this to add support for native histograms:

func (t *transaction) AppendHistogram(_ storage.SeriesRef, _ labels.Labels, _ int64, _ *histogram.Histogram, _ *histogram.FloatHistogram) (storage.SeriesRef, error) {
//TODO: implement this func
return 0, nil
}

I haven't tried it, so it may not be enough... There may be other config or something we need to change for the prometheus server to support it.

I'm OK adding support for native histograms, as long as it is behind an alpha feature gate (disabled by default) until the prometheus server itself promotes native histogram support out of experimental and (presumably) stabilizes the native histogram format.

Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

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

@github-actions github-actions bot added the Stale label Nov 13, 2023
@dashpole dashpole removed the Stale label Nov 13, 2023
codeboten pushed a commit that referenced this issue Nov 15, 2023
…negotiation (#29153)

The code needs some basic tests that can be later expanded with tests
for native histograms use cases.

Changes:
Refactored `testComponent` function to be easier to customize the
configuration of the scrape.
Expanded `compareHistogram` to assert on the explicit boundaries as
well.
Added function `prometheusMetricFamilyToProtoBuf` to helpers to be able
to turn serialize a Prometheus metric family
into Protobuf.
Added simple test of Protobuf based scrape of counters, gauges,
summaries and histograms.

#26555  

Followup to #27030 
Related to #28663 

**Testing:**

Adding simple e2e test for scraping over Protobuf. 

**Documentation:** 

Not applicable.

---------

Signed-off-by: György Krajcsovits <[email protected]>
Co-authored-by: David Ashpole <[email protected]>
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this issue Nov 24, 2023
…negotiation (open-telemetry#29153)

The code needs some basic tests that can be later expanded with tests
for native histograms use cases.

Changes:
Refactored `testComponent` function to be easier to customize the
configuration of the scrape.
Expanded `compareHistogram` to assert on the explicit boundaries as
well.
Added function `prometheusMetricFamilyToProtoBuf` to helpers to be able
to turn serialize a Prometheus metric family
into Protobuf.
Added simple test of Protobuf based scrape of counters, gauges,
summaries and histograms.

open-telemetry#26555  

Followup to open-telemetry#27030 
Related to open-telemetry#28663 

**Testing:**

Adding simple e2e test for scraping over Protobuf. 

**Documentation:** 

Not applicable.

---------

Signed-off-by: György Krajcsovits <[email protected]>
Co-authored-by: David Ashpole <[email protected]>
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

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

@github-actions github-actions bot added the Stale label Jan 15, 2024
@jmichalek132
Copy link
Contributor

Not stale being worked on in #28663

@codeboten codeboten removed the Stale label Jan 15, 2024
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

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

@github-actions github-actions bot added the Stale label Mar 18, 2024
jpkrohling pushed a commit that referenced this issue Apr 9, 2024
**Description:** 

Implement native histogram append MVP.
Very similar to appending a float sample.

Limitations:
- Only support integer counter histograms fully.
- In case a histogram has both classic and native buckets, we only store
one of them. Governed by scrape_classic_histograms scrape option. The
reason is that in the OTEL model the metric family is identified by the
normalized name (without _count, _sum, _bucket suffixes for the classic
histograms), meaning that the classic and native histograms would map to
the same metric family in OTEL model , but that cannot have both
Histogram and ExponentialHistogram types at the same time.
- Gauge histograms are dropped with warning as that temporality is
unsupported, see
open-telemetry/opentelemetry-specification#2714
- NoRecordedValue attribute might be unreliable. Prometheus scrape marks
all series with float NaN values when stale, but transactions in
prometheusreceiver are stateless, meaning that we have to use heuristics
to figure out if we need to add a NoRecordedValue data point to an
Exponential Histogram metric. (Need work in Prometheus.)



Additionally: 
- Created timestamp supported.
- Float counter histograms not fully tested and lose precision, but we
don't expect instrumentation to expose these anyway.

**Link to tracking Issue:**

Fixes: #26555 

**Testing:** 

Added unit tests and e2e tests.

**Documentation:**

TBD: will have to call out protobuf negotiation while no text format.
#27030

---------

Signed-off-by: György Krajcsovits <[email protected]>
Co-authored-by: David Ashpole <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority:needed Triagers reviewed the issue but need code owner to set priority receiver/prometheus Prometheus receiver Stale
Projects
None yet
5 participants