compact: native histogram support for downsampling #6350

rabenhorst · 2023-05-09T12:35:32Z

This PR adds native histogram support for downsampling in compact and is part of #5907.

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

Changes

Add native histogram support for downsampling

Verification

Add unit tests for native histograms in downsampling

GiedriusS · 2023-07-19T06:50:43Z

pkg/compact/downsample/downsample.go

+	}
+
+	if h.total > 0 {
+		if fh.DetectReset(h.previous) {


The increase of schema is treated like a reset:

A decrease of the ZeroThreshold or an increase of the Schema (i.e. an increase of resolution) can only happen together with a reset. Thus, the method returns true in either case.

If we do fh.CopyToSchema(h.schema) previously then this case is not caught AFAICT. Shouldn't we do the same reset logic in the preceding if?

I have to check that, but not sure if this will be still relevant. I proposed a change to how we can do downsampling: #6554

I added downsampling support for mixed chunk series and this PR is still open for review since #6554 will be a longer term task.

If we do fh.CopyToSchema(h.schema) previously then this case is not caught AFAICT. Shouldn't we do the same reset logic in the preceding if?

I think you are right and will look into it.

If we do fh.CopyToSchema(h.schema) previously then this case is not caught AFAICT. Shouldn't we do the same reset logic in the preceding if?

h.schema is the precalculated minimal schema from all samples of the series, which we need to know, otherwise we might run in the situation where we can't add a sample to aggregates. For the reset we need to compare current and previous histogram, so I suggest to keep the original histogram and use that for reset detection: d7d2224

From what I understand the head appender sets reset hints properly, so even without the fix it should work.

Signed-off-by: Sebastian Rabenhorst <[email protected]> Fixed imports Signed-off-by: Sebastian Rabenhorst <[email protected]> Added native histogram support for downsampling Signed-off-by: Sebastian Rabenhorst <[email protected]> Improved comment Signed-off-by: Sebastian Rabenhorst <[email protected]> Removed unnecessary if Signed-off-by: Sebastian Rabenhorst <[email protected]> Fixed native histogram proto conversion in remote engine Signed-off-by: Sebastian Rabenhorst <[email protected]> Added downsampling support for native histograms Reverted files from base PR Added changelog Signed-off-by: Sebastian Rabenhorst <[email protected]> Reverted error in bucket.go Signed-off-by: Sebastian Rabenhorst <[email protected]> trigger tests Signed-off-by: Sebastian Rabenhorst <[email protected]>

Signed-off-by: Sebastian Rabenhorst <[email protected]> Fixed tests, removed unused code Signed-off-by: Sebastian Rabenhorst <[email protected]> Fixed tests, removed unused code Signed-off-by: Sebastian Rabenhorst <[email protected]>

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

Signed-off-by: Sebastian Rabenhorst <[email protected]> Improved mixed type chunks and native histogram downsampling tests Signed-off-by: Sebastian Rabenhorst <[email protected]>

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

GiedriusS

We get this feedback consistently that Thanos is hard to understand and we are adding quite new functionality here thus could you please add a paragraph to docs/components/compact.md about how native histogram downsampling works?

GiedriusS · 2023-08-29T07:43:29Z

CHANGELOG.md

@@ -85,6 +85,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 - [#6163](https://github.com/thanos-io/thanos/pull/6163) Receiver: changed default max backoff from 30s to 5s for forwarding requests. Can be configured with `--receive-forward-max-backoff`.
 - [#6327](https://github.com/thanos-io/thanos/pull/6327) *: *breaking :warning:* Use histograms instead of summaries for instrumented handlers.
 - [#6322](https://github.com/thanos-io/thanos/pull/6322) Logging: Avoid expensive log.Valuer evaluation for disallowed levels.
+- [#6350] https://github.com/thanos-io/thanos/pull/6350 Compact: Add native histogram support for downsampling.


Suggested change

- [#6350] https://github.com/thanos-io/thanos/pull/6350 Compact: Add native histogram support for downsampling.

- [#6350](https://github.com/thanos-io/thanos/pull/6350) Compact: Add native histogram support for downsampling.

GiedriusS · 2023-08-29T07:45:33Z

pkg/store/storepb/testutil/series.go

@@ -53,6 +53,7 @@ type HeadGenOptions struct {
 	ScrapeInterval           time.Duration

 	WithWAL       bool
+	AppendLabels  labels.Labels


Is this used anywhere?

GiedriusS · 2023-08-29T07:55:28Z

pkg/store/bucket_test.go

+	// Native histograms only have aggregates for COUNT, COUNTER and SUM.
+	aggrsTypes := []storepb.Aggr{storepb.Aggr_COUNT, storepb.Aggr_COUNTER, storepb.Aggr_SUM}
+	for _, concurrency := range []int{1, 2, 4, 8, 16, 32} {
+		b.Run(fmt.Sprintf("aggregates: %v, concurrency: %d", aggrs, concurrency), func(b *testing.B) {


You probably meant to use aggrsTypes here? aggrs is empty 😄

GiedriusS · 2023-08-29T08:10:19Z

pkg/compact/downsample/downsample.go

@@ -179,23 +192,44 @@ func Downsample(
 							}
 						}
 					}
-
+				}
+				if i > 0 && previousIsHistogram != isHistogram(ac) {


Maybe downsampleAggr could handle this interleaving of different chunk types? For example, we could loop through the given data and perform downsampling individually for different sets of chunk types. It's getting hard to understand this loop. The same could probably be said about DownsampleRaw but there are fewer if branches there so it's understandable.

GiedriusS · 2023-08-29T08:32:42Z

pkg/compact/downsample/downsample.go

+	}
+
+	if h.sum == nil {
+		if h.resetDetected {


Don't we need to reset this to false?

GiedriusS · 2023-08-29T08:32:51Z

pkg/compact/downsample/downsample.go

+		} else {
+			h.sum = fh.Copy()
+		}
+		// This needs to be h gauge histogram, otherwise reset detection will be triggered


Suggested change

// This needs to be h gauge histogram, otherwise reset detection will be triggered

// This needs to be a gauge histogram, otherwise reset detection will be triggered

GiedriusS · 2023-08-29T08:34:12Z

pkg/compact/downsample/downsample.go

+		}
+		// This needs to be h gauge histogram, otherwise reset detection will be triggered
+		// when appending the aggregated chunk and histogram.count < appender.count.
+		h.sum.CounterResetHint = histogram.GaugeType


Isn't the result that this is always set when adding multiple histograms? Maybe we can move this out of the if?

GiedriusS · 2023-08-29T08:41:10Z

pkg/compact/downsample/downsample.go

+	b.chunks[AggrCount] = chunkenc.NewXORChunk()
+
+	// Sum aggregate always needs to be gauge type, otherwise
+	// append can fail when sum_window_n > sum_window_n+1 (.i.e


Suggested change

// append can fail when sum_window_n > sum_window_n+1 (.i.e

// append can fail when sum_window_n > sum_window_n+1 (i.e.

GiedriusS · 2023-08-29T08:43:41Z

pkg/compact/downsample/downsample.go

+		switch fh.CounterResetHint {
+		case histogram.GaugeType:
+			if app != nil {
+				var ()


Suggested change

var ()

rabenhorst · 2023-08-29T08:54:14Z

We get this feedback consistently that Thanos is hard to understand and we are adding quite new functionality here thus could you please add a paragraph to docs/components/compact.md about how native histogram downsampling works?

Agreed. I will ad documentation about it.

GiedriusS · 2024-02-22T12:01:19Z

Any update on this? Would be cool to merge it.

Matroxt · 2024-06-05T00:06:37Z

This seems to be the last piece missing to fully support native histograms in Thanos.
Can we help in any way so this can get merged?

GiedriusS · 2024-06-05T08:19:54Z

This seems to be the last piece missing to fully support native histograms in Thanos. Can we help in any way so this can get merged?

You can open up a separate PR using commits from this branch and then add extra fixes on top

rabenhorst · 2024-06-05T09:03:47Z

This seems to be the last piece missing to fully support native histograms in Thanos. Can we help in any way so this can get merged?

I don't had/have the time continue working on this. As @GiedriusS wrote, feel free to jut copy my branch. If you have question you can put them here or find me in CNCF slack.

pull-request-size bot added the size/XXL label May 9, 2023

rabenhorst changed the title ~~Native histogram support for downsampling (w.i.p.)~~ compact: native histogram support for downsampling (w.i.p.) May 9, 2023

rabenhorst mentioned this pull request May 9, 2023

Add Native Histogram Support #5907

Open

7 tasks

rabenhorst force-pushed the native-histogram-support-downsampling branch 4 times, most recently from 161bfb0 to 669417a Compare May 10, 2023 13:39

rabenhorst changed the title ~~compact: native histogram support for downsampling (w.i.p.)~~ compact: native histogram support for downsampling May 10, 2023

rabenhorst marked this pull request as ready for review May 10, 2023 15:54

rabenhorst force-pushed the native-histogram-support-downsampling branch from dc62091 to f049b1c Compare May 16, 2023 11:25

rabenhorst force-pushed the native-histogram-support-downsampling branch 4 times, most recently from ed2aa80 to bdf8e80 Compare June 13, 2023 12:55

douglascamata added the component: compact label Jun 13, 2023

rabenhorst force-pushed the native-histogram-support-downsampling branch from bdf8e80 to fd82b1a Compare July 10, 2023 18:33

GiedriusS reviewed Jul 19, 2023

View reviewed changes

rabenhorst mentioned this pull request Jul 24, 2023

compact: use thanos engine or matrix selector for downampling #6554

Open

rabenhorst force-pushed the native-histogram-support-downsampling branch from 619d806 to 0415cc4 Compare July 24, 2023 09:40

rabenhorst force-pushed the native-histogram-support-downsampling branch 2 times, most recently from 3b58955 to 97a8dd6 Compare August 1, 2023 10:50

rabenhorst force-pushed the native-histogram-support-downsampling branch from 55ae8d5 to c0cfcc6 Compare August 19, 2023 17:44

rabenhorst added 7 commits August 19, 2023 19:45

Fixed tests, removed unused code

c4b710e

Signed-off-by: Sebastian Rabenhorst <[email protected]> Fixed tests, removed unused code Signed-off-by: Sebastian Rabenhorst <[email protected]> Fixed tests, removed unused code Signed-off-by: Sebastian Rabenhorst <[email protected]>

Added test for mixed types and gauge reset fix

e613a35

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

Added mixed chunk type downsampling

e47ded9

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

Improved mixed type chunks and native histogram downsampling tests

0f1712e

Signed-off-by: Sebastian Rabenhorst <[email protected]> Improved mixed type chunks and native histogram downsampling tests Signed-off-by: Sebastian Rabenhorst <[email protected]>

Cleanup

0fa8ac5

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

Fixes after rebase

c402aaa

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

rabenhorst force-pushed the native-histogram-support-downsampling branch from c0cfcc6 to c402aaa Compare August 19, 2023 17:45

Fixed counter reset detection for histogram downsampling.

d7d2224

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

GiedriusS reviewed Aug 29, 2023

View reviewed changes

rabenhorst mentioned this pull request Dec 18, 2023

Crash in 0.31.0 in compactor: panic: cannot call histogramIterator. #6983

Open

yeya24 mentioned this pull request Sep 16, 2024

Native Histograms: Downsampling support #7747

Open

GiedriusS mentioned this pull request Nov 25, 2024

Remote write 2.0 support #7935

Open

GiedriusS mentioned this pull request Feb 17, 2025

compact: implement native histogram downsampling #8110

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compact: native histogram support for downsampling #6350

compact: native histogram support for downsampling #6350

rabenhorst commented May 9, 2023 •

edited

Loading

GiedriusS Jul 19, 2023

rabenhorst Jul 24, 2023

rabenhorst Aug 1, 2023

rabenhorst Aug 1, 2023

rabenhorst Aug 19, 2023

GiedriusS left a comment

GiedriusS Aug 29, 2023

GiedriusS Aug 29, 2023

GiedriusS Aug 29, 2023

GiedriusS Aug 29, 2023

GiedriusS Aug 29, 2023

GiedriusS Aug 29, 2023

GiedriusS Aug 29, 2023

GiedriusS Aug 29, 2023

GiedriusS Aug 29, 2023

rabenhorst commented Aug 29, 2023

GiedriusS commented Feb 22, 2024

Matroxt commented Jun 5, 2024

GiedriusS commented Jun 5, 2024

rabenhorst commented Jun 5, 2024

	- [#6350] https://github.com/thanos-io/thanos/pull/6350 Compact: Add native histogram support for downsampling.
	- [#6350](https://github.com/thanos-io/thanos/pull/6350) Compact: Add native histogram support for downsampling.

	// This needs to be h gauge histogram, otherwise reset detection will be triggered
	// This needs to be a gauge histogram, otherwise reset detection will be triggered

	// append can fail when sum_window_n > sum_window_n+1 (.i.e
	// append can fail when sum_window_n > sum_window_n+1 (i.e.

compact: native histogram support for downsampling #6350

Are you sure you want to change the base?

compact: native histogram support for downsampling #6350

Conversation

rabenhorst commented May 9, 2023 • edited Loading

Changes

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabenhorst commented Aug 29, 2023

GiedriusS commented Feb 22, 2024

Matroxt commented Jun 5, 2024

GiedriusS commented Jun 5, 2024

rabenhorst commented Jun 5, 2024

rabenhorst commented May 9, 2023 •

edited

Loading