Implement Native histogram Binary operations and Change Functions for Compatibility with v3.1.0 #503

SungJin1212 · 2025-01-03T11:06:10Z

Implement missing part to address these PRs.

PromQL engine: Delay deletion of __name__ label to the end of the query evaluation prometheus/prometheus#14477 - Delayed name dropping
Moves TestNativeHistogram_MulDivOperator to promql testing framework prometheus/prometheus#14688 - Moved the native histogram binary operation tests into acceptance tests.
promql: fix incorrect results and panics in sum and avg over mixed custom and exponential buckets, or incompatible custom buckets prometheus/prometheus#14611 and more - Added annotations to native histogram

Update

Update the Prometheus version to v3.1.0 to this PR: Fix round function not honoring delayed name removal flag prometheus/prometheus#15250)

Implement v3.1.0 Prometheus PRs

The Delayed name dropping implementation exists in another PR.

Implement delayed name removal #507

pedro-stanaka · 2025-01-03T13:05:32Z

I would suggest updating the title to better reflect what you are actually doing. Are those PRs part of the Prometheus 3.0 update?

SungJin1212 · 2025-01-03T13:48:11Z

Yeah, this PR is for the Prometheus 3.0 update.

MichaHoffmann · 2025-01-04T19:58:19Z

execution/aggregate/accumulator.go

@@ -389,6 +406,14 @@ func (a *avgAcc) AddVector(vs []float64, hs []*histogram.FloatHistogram) error {
 	}
 	for _, h := range hs {
 		if err := a.Add(0, h); err != nil {
+			if errors.Is(err, histogram.ErrHistogramsIncompatibleSchema) {
+				// skip warning


Why dont we also write a warning here ( and in the other cases )?

At the Prometheus it is a warning but we get the fail if we return the error. So, I temporality return nil.

You can add warnings too here - we have a package for it "execution/warning" I think

ringbuffer/functions.go

MichaHoffmann

I think delayed name removal has couple bugs; as some functions are not implemented in the function operator and either have special operators or are pushed into the vector operator (like timestamp) or matrix operator (like any range vector function). I would vote that we ignore delayed name removal for now since its behind feature flag in prometheus anyway. If its decided that its going to be removed we have spent effort here that we need to roll back later. Otherwise looks great, thank you for the effort!

SungJin1212 · 2025-01-05T01:12:27Z

@MichaHoffmann
Yes, It's a draft PR, and I gradually update it under some decisions. :D

SungJin1212 · 2025-01-06T07:01:29Z

@MichaHoffmann
I think we need to upgrade the Prometheus to 3.1.0 to apply prometheus/prometheus#15250, and some package structures have been changed.

harry671003 · 2025-01-06T23:08:05Z

@SungJin1212 - Could you also change the branch name in the workflow configuration to update-prometheus-3.0, so that it'll trigger the tests?

https://github.com/thanos-io/promql-engine/blob/update-prometheus-3.0/.github/workflows/test.yaml#L7

MichaHoffmann · 2025-01-07T20:00:37Z

@MichaHoffmann
I think we need to upgrade the Prometheus to 3.1.0 to apply prometheus/prometheus#15250, and some package structures have been changed.

I think should not implement the delayed name removal flag here yet! Or at least it should be a separate PR but I think we should not do it unless it's the default in Prometheus

yeya24 · 2025-01-08T05:12:53Z

@SungJin1212 Maybe let's do what @MichaHoffmann to temporarily remove the delayed name removal flag.

If the only failed test is warning and info comparison, I think we are ok to merge to unblock 3.0 upgrade even if it failed the CI.

MichaHoffmann · 2025-01-08T06:50:35Z

execution/aggregate/accumulator.go

@@ -287,6 +309,7 @@ func (c *countAcc) Reset(_ float64) {
 }

 type avgAcc struct {
+	ctx         context.Context


can we just add it to the "Add" and "AddVector" method as first argument instead as a struct field of the accumulator?

MichaHoffmann · 2025-01-08T12:45:42Z

execution/aggregate/vector_table.go

@@ -50,7 +52,7 @@ func (t *vectorTable) timestamp() int64 {

 func (t *vectorTable) aggregate(vector model.StepVector) error {


can we add ctx here instead of using it as a struct field?

SungJin1212 · 2025-01-09T02:11:32Z

execution/scan/subquery.go

 			if err != nil {
 				return nil, err
 			}
 			if ok {
 				if h != nil {
 					sv.AppendHistogram(o.pool, uint64(sampleId), h)
-				} else {
-					sv.AppendSample(o.pool, uint64(sampleId), f)
+				} else if f != nil {


@MichaHoffmann
I made the float64 to pointer to skip the value append when the engine should append nothing.

Signed-off-by: SungJin1212 <[email protected]>

MichaHoffmann · 2025-01-15T06:01:37Z

execution/binary/vector.go

@@ -12,6 +12,8 @@ import (

 	"github.com/cespare/xxhash/v2"
 	"github.com/efficientgo/core/errors"
+	"github.com/prometheus/prometheus/promql/parser/posrange"


can we group with the other prometheus imports?

MichaHoffmann · 2025-01-15T06:03:19Z

execution/binary/vector.go

 		if err != nil {
-			return model.StepVector{}, err
+			if errors.Is(err, annotations.PromQLInfo) || errors.Is(err, annotations.PromQLWarning) {


why does computeBinaryPairing produce an "info/warning" error? that should not happen i think

If the vectorElemBinop return err, it produces err containing info/warnings.

The reason why use continue is to ignore append. The current implementation return model.StepVector{}, nil, then it appends 0.
We get the fail like this test caes:

eval_info range from 0 to 24m step 6m left_histograms == bool right_floats_for_histograms # No results.

MichaHoffmann · 2025-01-15T06:05:43Z

execution/binary/vector.go

+			case parser.DIV:
+				return 0, hlhs.Copy().Div(rhs).Compact(0), true, nil
+			case parser.ADD, parser.SUB, parser.POW, parser.MOD, parser.EQLC, parser.NEQ, parser.GTR, parser.LSS, parser.GTE, parser.LTE, parser.ATAN2:
+				err := annotations.NewIncompatibleTypesInBinOpInfo("histogram", opName, "float", posrange.PositionRange{})


I dont think we should return an error and an annotation at this point; that feels redundant.

Prometheus also does it, but can we at least add the annotations somewhere else then?

harry671003 · 2025-01-15T17:49:57Z

engine/sort.go

@@ -7,6 +7,7 @@ import (
 	"math"

 	"github.com/facette/natsort"
+


nit: Some of these files contain changes that are not relevant like adding whitespace or re-ordering the imports. Could you remove them?

Signed-off-by: SungJin1212 <[email protected]>

SungJin1212 force-pushed the Implement-missing-parts branch 6 times, most recently from 4018238 to c3a5bbd Compare January 3, 2025 12:14

SungJin1212 marked this pull request as draft January 3, 2025 13:48

SungJin1212 changed the title ~~Implement missing parts~~ Implement Delayed name dropping and Native histogram Jan 3, 2025

SungJin1212 changed the title ~~Implement Delayed name dropping and Native histogram~~ Implement Delayed name dropping and Native histogram Binary operations Jan 3, 2025

harry671003 requested a review from yeya24 January 4, 2025 05:40

MichaHoffmann reviewed Jan 4, 2025

View reviewed changes

ringbuffer/functions.go Outdated Show resolved Hide resolved

MichaHoffmann reviewed Jan 4, 2025

View reviewed changes

SungJin1212 force-pushed the Implement-missing-parts branch from c3a5bbd to 9b14ea6 Compare January 5, 2025 01:13

SungJin1212 marked this pull request as ready for review January 6, 2025 01:51

MichaHoffmann reviewed Jan 8, 2025

View reviewed changes

SungJin1212 commented Jan 9, 2025

View reviewed changes

SungJin1212 force-pushed the Implement-missing-parts branch 2 times, most recently from 9d2e6ce to 1c3604e Compare January 13, 2025 07:57

SungJin1212 changed the title ~~Implement Delayed name dropping and Native histogram Binary operations~~ Implement Native histogram Binary operations and Change Functions for Compatibility with v3.1.0 Jan 13, 2025

SungJin1212 force-pushed the Implement-missing-parts branch from 741014f to a904f7b Compare January 14, 2025 02:16

Update prometheus version to v3.1.0

6c893ed

Signed-off-by: SungJin1212 <[email protected]>

SungJin1212 added 16 commits January 14, 2025 11:27

Add ctx for adding histogram warning

9b6203b

Signed-off-by: SungJin1212 <[email protected]>

Add histogram address logic to stdDev, stdVar

753b09b

Signed-off-by: SungJin1212 <[email protected]>

Add HistogramIgnoredInAggregationInfo to quantile accumulator

497f9f0

Signed-off-by: SungJin1212 <[email protected]>

Change count_values func applying histogram

e6bd19b

Signed-off-by: SungJin1212 <[email protected]>

Add warning to kAggregate

9cc7f6c

Signed-off-by: SungJin1212 <[email protected]>

Implement histogram binops

653803f

Signed-off-by: SungJin1212 <[email protected]>

Add NewPossibleNonCounterInfo

5fd39e2

Signed-off-by: SungJin1212 <[email protected]>

Add histogram address login to some funcs

ffc03e8

Signed-off-by: SungJin1212 <[email protected]>

Add NewHistogramQuantileForcedMonotonicityInfo

2be039d

Signed-off-by: SungJin1212 <[email protected]>

Change bucketQuantile to return forcedMonotonic

5070818

Signed-off-by: SungJin1212 <[email protected]>

Add labelReplaceDst validation

d5ee087

Signed-off-by: SungJin1212 <[email protected]>

Change float64 to pointer to append nothing

3208215

Signed-off-by: SungJin1212 <[email protected]>

Add histogram address logic to sum, max over time

4074f07

Signed-off-by: SungJin1212 <[email protected]>

Add histogram address logic to linearRegression

e6dd9e6

Signed-off-by: SungJin1212 <[email protected]>

Add histogram address logic to resets

d094a47

Signed-off-by: SungJin1212 <[email protected]>

Add warning to histogramRate

f99e6db

Signed-off-by: SungJin1212 <[email protected]>

SungJin1212 force-pushed the Implement-missing-parts branch from a904f7b to f99e6db Compare January 14, 2025 02:39

MichaHoffmann reviewed Jan 15, 2025

View reviewed changes

harry671003 reviewed Jan 15, 2025

View reviewed changes

group prometheus imports

85b9e94

Signed-off-by: SungJin1212 <[email protected]>

SungJin1212 force-pushed the Implement-missing-parts branch from f60cb6c to 85b9e94 Compare January 16, 2025 02:20

Refactor vector.go to merge annotations

5d75bd3

Signed-off-by: SungJin1212 <[email protected]>

SungJin1212 force-pushed the Implement-missing-parts branch from c2252cf to 5d75bd3 Compare January 16, 2025 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Native histogram Binary operations and Change Functions for Compatibility with v3.1.0 #503

Implement Native histogram Binary operations and Change Functions for Compatibility with v3.1.0 #503

SungJin1212 commented Jan 3, 2025 •

edited

Loading

pedro-stanaka commented Jan 3, 2025

SungJin1212 commented Jan 3, 2025

MichaHoffmann Jan 4, 2025

SungJin1212 Jan 5, 2025

MichaHoffmann Jan 5, 2025

MichaHoffmann left a comment •

edited

Loading

SungJin1212 commented Jan 5, 2025

SungJin1212 commented Jan 6, 2025

harry671003 commented Jan 6, 2025

MichaHoffmann commented Jan 7, 2025

yeya24 commented Jan 8, 2025

MichaHoffmann Jan 8, 2025

MichaHoffmann Jan 8, 2025

SungJin1212 Jan 9, 2025

MichaHoffmann Jan 15, 2025

MichaHoffmann Jan 15, 2025

SungJin1212 Jan 15, 2025 •

edited

Loading

MichaHoffmann Jan 15, 2025

MichaHoffmann Jan 15, 2025

harry671003 Jan 15, 2025

		@@ -50,7 +52,7 @@ func (t *vectorTable) timestamp() int64 {

		func (t *vectorTable) aggregate(vector model.StepVector) error {

Implement Native histogram Binary operations and Change Functions for Compatibility with v3.1.0 #503

Are you sure you want to change the base?

Implement Native histogram Binary operations and Change Functions for Compatibility with v3.1.0 #503

Conversation

SungJin1212 commented Jan 3, 2025 • edited Loading

pedro-stanaka commented Jan 3, 2025

SungJin1212 commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaHoffmann left a comment • edited Loading

Choose a reason for hiding this comment

SungJin1212 commented Jan 5, 2025

SungJin1212 commented Jan 6, 2025

harry671003 commented Jan 6, 2025

MichaHoffmann commented Jan 7, 2025

yeya24 commented Jan 8, 2025

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

SungJin1212 Jan 15, 2025 • edited Loading

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

SungJin1212 commented Jan 3, 2025 •

edited

Loading

MichaHoffmann left a comment •

edited

Loading

SungJin1212 Jan 15, 2025 •

edited

Loading