sidecar, query, receiver: Native histogram support #6032

rabenhorst · 2023-01-10T11:26:46Z

Add native histogram support for sidecar, query, receiver. It does not include necessary changes for query UI and query frontend. The plan is to do this separate PRs. Relates to #5907.

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Added native histogram to prompb and storepb types (including helper/conversion functions)
Implemented native histogram appending in receiver writer
Implemented native histogram support for related iterators

Verification

Native histogram e2e tests to test query (including dedup) and write through sidecar and receiver
- Exposed raw json message in QueryInstant of promclient as workaround (until Add histogram vectors to model prometheus/common#417 is available)
Added test and benchmark for native histogram for receiver write

pkg/dedup/iter.go

docs/proposals-accepted/202301-distributed-query-execution.md

cmd/thanos/receive.go

pkg/store/prometheus.go

fpetkovski

Looks good, thanks. Just one comment from my side

test/e2e/native_histograms_test.go

yeya24 · 2023-01-13T06:03:49Z

pkg/query/querier_test.go

@@ -843,7 +843,6 @@ func (s *mockedSeriesIterator) At() (t int64, v float64) {
 	return sample.t, sample.v
 }

-// TODO(rabenhorst): Needs to be implemented for native histogram support.
 func (s *mockedSeriesIterator) AtHistogram() (int64, *histogram.Histogram) {


I see we still panic in this method but removed the comment. Do we still need to implement anything?

I will restore the comment. Rn mockedSeriesIterator does not support histograms and if we need it it needs to be implemented.

cmd/thanos/receive.go

rabenhorst · 2023-01-13T20:15:24Z

I updated prometheus/common so that it includes prometheus/common#417, which is not tagged yet but helps to improve tests (see last commit).

fpetkovski

Thanks, this looks good now 👍

GiedriusS

LGTM, just a nit

pkg/query/iter.go

GiedriusS · 2023-01-24T10:20:21Z

pkg/dedup/iter.go

 	lastUseA := it.useA
 	defer func() {
-		if it.useA != lastUseA {
+		if it.useA != lastUseA && isFloatVal {


adjustAtValue seems to already be protected against doing something with non-float values but this makes code clearer 👍

Signed-off-by: Sebastian Rabenhorst <[email protected]> Added comment Signed-off-by: Sebastian Rabenhorst <[email protected]> Cleanup native histogram tests Signed-off-by: Sebastian Rabenhorst <[email protected]> Made native hist test ha and replace usage of at with att Signed-off-by: Sebastian Rabenhorst <[email protected]> Fix deduplication with native histograms Signed-off-by: Sebastian Rabenhorst <[email protected]> Enabled native histogram writing Signed-off-by: Sebastian Rabenhorst <[email protected]> Committed missing files for write Signed-off-by: Sebastian Rabenhorst <[email protected]> Added benchmark for histogram writing Signed-off-by: Sebastian Rabenhorst <[email protected]> Fixed chunkSeriesIterator Signed-off-by: Sebastian Rabenhorst <[email protected]> Enabled replication for native histogram write test Signed-off-by: Sebastian Rabenhorst <[email protected]> Lint native histogram test imports Signed-off-by: Sebastian Rabenhorst <[email protected]> Removed ToDo comments Signed-off-by: Sebastian Rabenhorst <[email protected]> Cleanup Signed-off-by: Sebastian Rabenhorst <[email protected]>

Signed-off-by: Sebastian Rabenhorst <[email protected]>

* Added native histogram support for sidecar, query, receiver. Signed-off-by: Sebastian Rabenhorst <[email protected]> Added comment Signed-off-by: Sebastian Rabenhorst <[email protected]> Cleanup native histogram tests Signed-off-by: Sebastian Rabenhorst <[email protected]> Made native hist test ha and replace usage of at with att Signed-off-by: Sebastian Rabenhorst <[email protected]> Fix deduplication with native histograms Signed-off-by: Sebastian Rabenhorst <[email protected]> Enabled native histogram writing Signed-off-by: Sebastian Rabenhorst <[email protected]> Committed missing files for write Signed-off-by: Sebastian Rabenhorst <[email protected]> Added benchmark for histogram writing Signed-off-by: Sebastian Rabenhorst <[email protected]> Fixed chunkSeriesIterator Signed-off-by: Sebastian Rabenhorst <[email protected]> Enabled replication for native histogram write test Signed-off-by: Sebastian Rabenhorst <[email protected]> Lint native histogram test imports Signed-off-by: Sebastian Rabenhorst <[email protected]> Removed ToDo comments Signed-off-by: Sebastian Rabenhorst <[email protected]> Cleanup Signed-off-by: Sebastian Rabenhorst <[email protected]> * Renamed lastValue of dedup iter and added TODO Signed-off-by: Sebastian Rabenhorst <[email protected]> * Fixed typo Signed-off-by: Sebastian Rabenhorst <[email protected]> * Added hidden native histogram flag for receive tsdb Signed-off-by: Sebastian Rabenhorst <[email protected]> * merge Signed-off-by: Sebastian Rabenhorst <[email protected]> * Comments and reverted change to qfe Signed-off-by: Sebastian Rabenhorst <[email protected]> * Go mod tidy Signed-off-by: Sebastian Rabenhorst <[email protected]> * Dedup iter nit Signed-off-by: Sebastian Rabenhorst <[email protected]> Signed-off-by: Sebastian Rabenhorst <[email protected]>

pull-request-size bot added the size/XL label Jan 10, 2023

rabenhorst force-pushed the native-histograms branch 2 times, most recently from 831ee0c to 67a4190 Compare January 10, 2023 11:37

fpetkovski reviewed Jan 10, 2023

View reviewed changes

pkg/dedup/iter.go Show resolved Hide resolved

rabenhorst marked this pull request as ready for review January 10, 2023 14:25

rabenhorst force-pushed the native-histograms branch from 5e05fb2 to 29fdb83 Compare January 10, 2023 17:09

yeya24 reviewed Jan 11, 2023

View reviewed changes

docs/proposals-accepted/202301-distributed-query-execution.md Outdated Show resolved Hide resolved

yeya24 mentioned this pull request Jan 11, 2023

Add Native Histogram Support #5907

Open

7 tasks

rabenhorst force-pushed the native-histograms branch from 29fdb83 to 0d35f4a Compare January 11, 2023 07:32

rabenhorst commented Jan 11, 2023

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

rabenhorst force-pushed the native-histograms branch 2 times, most recently from 57c9771 to 8edbf72 Compare January 12, 2023 13:57

fpetkovski reviewed Jan 12, 2023

View reviewed changes

pkg/store/prometheus.go Outdated Show resolved Hide resolved

fpetkovski reviewed Jan 12, 2023

View reviewed changes

yeya24 reviewed Jan 13, 2023

View reviewed changes

rabenhorst force-pushed the native-histograms branch 3 times, most recently from c938a32 to ca26c93 Compare January 16, 2023 16:07

fpetkovski previously approved these changes Jan 17, 2023

View reviewed changes

rabenhorst dismissed fpetkovski’s stale review via d61405a January 17, 2023 13:53

rabenhorst force-pushed the native-histograms branch from ca26c93 to d61405a Compare January 17, 2023 13:53

fpetkovski mentioned this pull request Jan 19, 2023

Support native histograms in store-gateway #6056

Merged

2 tasks

rabenhorst force-pushed the native-histograms branch 2 times, most recently from e3347d6 to dc81061 Compare January 19, 2023 12:10

fpetkovski previously approved these changes Jan 19, 2023

View reviewed changes

GiedriusS reviewed Jan 24, 2023

View reviewed changes

rabenhorst dismissed fpetkovski’s stale review via 4a5d074 January 24, 2023 11:01

rabenhorst force-pushed the native-histograms branch from 204b20d to 4109b40 Compare January 24, 2023 11:02

rabenhorst added 7 commits January 24, 2023 13:02

Renamed lastValue of dedup iter and added TODO

a67e913

Signed-off-by: Sebastian Rabenhorst <[email protected]>

Fixed typo

0f28067

Signed-off-by: Sebastian Rabenhorst <[email protected]>

Added hidden native histogram flag for receive tsdb

edfc9e6

Signed-off-by: Sebastian Rabenhorst <[email protected]>

merge

25e26aa

Signed-off-by: Sebastian Rabenhorst <[email protected]>

Comments and reverted change to qfe

44617d8

Signed-off-by: Sebastian Rabenhorst <[email protected]>

Go mod tidy

908330e

Signed-off-by: Sebastian Rabenhorst <[email protected]>

Dedup iter nit

6bc4626

Signed-off-by: Sebastian Rabenhorst <[email protected]>

rabenhorst force-pushed the native-histograms branch from 4b432ea to 6bc4626 Compare January 24, 2023 12:06

GiedriusS approved these changes Jan 24, 2023

View reviewed changes

GiedriusS merged commit 071b827 into thanos-io:main Jan 24, 2023

rabenhorst mentioned this pull request Jan 24, 2023

query frontend, query UI: Native histogram support #6071

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sidecar, query, receiver: Native histogram support #6032

sidecar, query, receiver: Native histogram support #6032

rabenhorst commented Jan 10, 2023 •

edited

Loading

fpetkovski left a comment

yeya24 Jan 13, 2023

rabenhorst Jan 16, 2023 •

edited

Loading

rabenhorst commented Jan 13, 2023

fpetkovski left a comment

GiedriusS left a comment

GiedriusS Jan 24, 2023

sidecar, query, receiver: Native histogram support #6032

sidecar, query, receiver: Native histogram support #6032

Conversation

rabenhorst commented Jan 10, 2023 • edited Loading

Changes

Verification

fpetkovski left a comment

Choose a reason for hiding this comment

yeya24 Jan 13, 2023

Choose a reason for hiding this comment

rabenhorst Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

rabenhorst commented Jan 13, 2023

fpetkovski left a comment

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

GiedriusS Jan 24, 2023

Choose a reason for hiding this comment

rabenhorst commented Jan 10, 2023 •

edited

Loading

rabenhorst Jan 16, 2023 •

edited

Loading