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

Add Native Histogram Support #5907

Open
6 of 7 tasks
rabenhorst opened this issue Nov 18, 2022 · 6 comments
Open
6 of 7 tasks

Add Native Histogram Support #5907

rabenhorst opened this issue Nov 18, 2022 · 6 comments

Comments

@rabenhorst
Copy link
Contributor

rabenhorst commented Nov 18, 2022

Is your proposal related to a problem?

Experimental native histogram support was released with Prometheus v0.40.x. #5896 updated Thanos to Prometheus v0.40.1 but without implementing native histograms.

I propose the implementation of native histogram support (behind a feature flag) as a next step in follow up PR(s).

Describe the solution you'd like

We need to go through all Thanos components and check what needs to be changed for native histogram support. So far we identified the following changes are required (list will be updated on new findings):

For each component we need to make sure to have tests in place after implementation and e2e test for functionality provided by multiple components (e.g. ingestion, querying, etc.).

@yeya24
Copy link
Contributor

yeya24 commented Jan 11, 2023

Just double checking, I see we are adding native histogram support to query, receiver and sidecar in #6032.

For components like compactor, store gateway, do we still need implement anything to make sure they work with native histograms? If no code changes required, at least we should create some tests to make sure they work properly.

Updated: NVM I think we need additional support for those components. Most of our code are assuming XOR encoding or Aggr encoding only.

@yeya24
Copy link
Contributor

yeya24 commented Feb 27, 2023

IIUC, only query frontend, Compactor and Ruler needs to support Native histograms?

I am wondering if we need to do anything specific in Ruler or it works out of the box by updating the Prometheus dependency. I guess the latter.

@rabenhorst
Copy link
Contributor Author

IIUC, only query frontend, Compactor and Ruler needs to support Native histograms?

I am wondering if we need to do anything specific in Ruler or it works out of the box by updating the Prometheus dependency. I guess the latter.

I also think it's the latter, but we will double check and add an e2e test for rulers.

@rabenhorst
Copy link
Contributor Author

rabenhorst commented Mar 30, 2023

Downsampling and compaction is blocked by a bug we hit in Prometheus regarding appending histograms to open chunks during compaction, which should be fixed by prometheus/prometheus#12185.
The bug needs to be fixed before we can open a PR for compaction/downsampling of native histograms.

@rabenhorst
Copy link
Contributor Author

rabenhorst commented Apr 11, 2023

Ruler does not work with native histograms out of the box. promclient returns model representation of native histogram for queries, which we need to translate to histogram.FloatHistograms:

vec = append(vec, promql.Sample{
Metric: lset,
Point: promql.Point{T: int64(e.Timestamp), V: float64(e.Value)},
})

We will look into using the GRPC query API for the ruler so we directly get histogram.FloatHistograms in the result.

@rabenhorst
Copy link
Contributor Author

We have PRs now for all components. I just added the last one for rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants