Repository: prometheus/prometheus (language: go)

Issue:
Annotations for incompatible native histogram operations with binary operators have confusing message
### What did you do?

I performed the following queries:

`series_with_exponential_native_histograms + series_with_custom_buckets_native_histograms`

`series_with_custom_buckets_native_histograms_layout_1 + series_with_custom_buckets_native_histograms_layout_2`

As the names suggest, these series contained native histograms with exponential and custom buckets.

### What did you expect to see?

Warnings that either include the appropriate metric names, or don't mention metric names at all.

For example, the first case could return a warning like `PromQL warning: vector contains a mix of histograms with exponential and custom buckets schemas for binary operation (1:1)`.

### What did you see instead? Under which circumstances?

The first query result included the warning `PromQL warning: vector contains a mix of histograms with exponential and custom buckets schemas for metric name "" (1:1)`.

The second query result included the warning `PromQL warning: vector contains histograms with incompatible custom buckets for metric name "" (1:1)`.

### System information

_No response_

### Prometheus version

_No response_

### Prometheus configuration file

_No response_

### Alertmanager version

_No response_

### Alertmanager configuration file

_No response_

### Logs

_No response_


Discussion / hints:
Clearly, the intention was to have the metric field filled.

@zenador would you like to investigate?
> Clearly, the intention was to have the metric field filled.

Which metric name would we use for binary operations though? The left side? The right? Both? (What happens if the left or right side don't have metric names?)

Ideally, we would have both names, or even be more specific about the exact label match that leads to the collision. Or maybe that's way too much noise and we should be more general. That needs to be explored.

In any case, it's not just about the name. The wording of the annotation ("vector contains a mix…") is just not right for bin-ops, where the mismatch is between elements from _different_ vectors.
Hi @charleskorn , @beorn7 . I investigated on this and I think [this](https://github.com/prometheus/prometheus/blob/d4994e5bc44490441a6a6ac05331cc6fbabae0f5/promql/engine.go#L3263) function handles all the error related to binary operations between vectors. Here deliberately the metric name is set to empty string, so that's the reason for empty metric name in the annotation.
Thanks for investigating.

Yeah, so at the very least, we would need a more generic wording in the current annotation (or more flexible wording if the metric name is empty). E.g. as discussed above, in the operator case, the mismatch is between elements in different vectors, not a mix within the same vector.

Alternatively, we use a new annotation for this case, and then we could investigate how to do the specific wording, maybe something like "label match {foo="bar", dings="bums"} combines a float and a histograms" or something.
Yeah, since these annotations are added in Vector binary operations error handler, we can introduce a new annotation for this case, like "binary operations is performed on histograms with exponential and custom buckets schemas (between {LHS expr} and {RHS expr})". 
What are your thoghts on this?
Yes, we should have a better fitting annotation. Not sure how detailed we want the message to be. If we enumerate all label matches that lead to a mix, we might end up with a lot of annotations. (On the other hand, we already limit the output, so maybe that's fine.)
@NeerajGartia21 since you added annotation for other bin ops, I'll assign this to you for now.
Thanks @beorn7 for assigning it to me.  I'll have a look over it.
For this Issue, I found three options.
- We can add the metric name to the current annotation like- `PromQL warning: vector contains a mix of histograms with exponential and custom buckets schemas for metric name "metricName" (1:1)`. But It is not well suited for binary operations and does not convey the message well.
- We can use the existing info annotation like- `incompatible sample types encountered for binary operator: exponential bucket histogram $OP custom bucket histogram`. It seems good but it is an info annotation and currently we are using warn annotation.
- Thirdly we can create another warn annotation like `$BINOP contains a mix of histogram with custom bucket and exponential bucket for the metric : $METRIC_NAME`

What are your opinions on these?
> We can use the existing info annotation like- incompatible sample types encountered for binary operator: exponential bucket histogram $OP custom bucket histogram. It seems good but it is an info annotation and currently we are using warn annotation.

If possible, I think we should use the same approach as for this bin-op info annotation, just on warn level and maybe change the text from "incompatible sample type" to "incompatible bucket layout". Also, if we have two custom bucket histograms, it wouldn't be very understandable to say "custom bucket histogram $OP custom bucket histogram". It had to be something like "incompatible custom bucket layout encountered for binary operator $OP". Or keep it simple and just say "incompatible custom bucket layout encountered for binary operator $OP" without explaining the arguments. I assume this will be rare enough that people should be fine with it.
Yeah, I think we can go with `incompatible bucket layout encountered for binary operator $OP`. It will be sufficient enough to convey the message ig.

Fix patch:
diff --git a/promql/engine.go b/promql/engine.go
index cf669282011..44985e50f90 100644
--- a/promql/engine.go
+++ b/promql/engine.go
@@ -3475,15 +3475,14 @@ func handleVectorBinopError(err error, e *parser.BinaryExpr) annotations.Annotat
 	if err == nil {
 		return nil
 	}
-	metricName := ""
+	op := parser.ItemTypeStr[e.Op]
 	pos := e.PositionRange()
 	if errors.Is(err, annotations.PromQLInfo) || errors.Is(err, annotations.PromQLWarning) {
 		return annotations.New().Add(err)
 	}
-	if errors.Is(err, histogram.ErrHistogramsIncompatibleSchema) {
-		return annotations.New().Add(annotations.NewMixedExponentialCustomHistogramsWarning(metricName, pos))
-	} else if errors.Is(err, histogram.ErrHistogramsIncompatibleBounds) {
-		return annotations.New().Add(annotations.NewIncompatibleCustomBucketsHistogramsWarning(metricName, pos))
+	// TODO(NeerajGartia21): Test the exact annotation output once the testing framework can do so.
+	if errors.Is(err, histogram.ErrHistogramsIncompatibleSchema) || errors.Is(err, histogram.ErrHistogramsIncompatibleBounds) {
+		return annotations.New().Add(annotations.NewIncompatibleBucketLayoutInBinOpWarning(op, pos))
 	}
 	return nil
 }
diff --git a/util/annotations/annotations.go b/util/annotations/annotations.go
index 5b2fde152bd..95783957a7e 100644
--- a/util/annotations/annotations.go
+++ b/util/annotations/annotations.go
@@ -143,6 +143,7 @@ var (
 	NativeHistogramNotGaugeWarning             = fmt.Errorf("%w: this native histogram metric is not a gauge:", PromQLWarning)
 	MixedExponentialCustomHistogramsWarning    = fmt.Errorf("%w: vector contains a mix of histograms with exponential and custom buckets schemas for metric name", PromQLWarning)
 	IncompatibleCustomBucketsHistogramsWarning = fmt.Errorf("%w: vector contains histograms with incompatible custom buckets for metric name", PromQLWarning)
+	IncompatibleBucketLayoutInBinOpWarning     = fmt.Errorf("%w: incompatible bucket layout encountered for binary operator", PromQLWarning)
 
 	PossibleNonCounterInfo                  = fmt.Errorf("%w: metric might not be a counter, name does not end in _total/_sum/_count/_bucket:", PromQLInfo)
 	HistogramQuantileForcedMonotonicityInfo = fmt.Errorf("%w: input to histogram_quantile needed to be fixed for monotonicity (see https://prometheus.io/docs/prometheus/latest/querying/functions/#histogram_quantile) for metric name", PromQLInfo)
@@ -295,9 +296,20 @@ func NewHistogramIgnoredInAggregationInfo(aggregation string, pos posrange.Posit
 	}
 }
 
+// NewHistogramIgnoredInMixedRangeInfo is used when a histogram is ignored
+// in a range vector which contains mix of floats and histograms.
 func NewHistogramIgnoredInMixedRangeInfo(metricName string, pos posrange.PositionRange) error {
 	return annoErr{
 		PositionRange: pos,
 		Err:           fmt.Errorf("%w %q", HistogramIgnoredInMixedRangeInfo, metricName),
 	}
 }
+
+// NewIncompatibleBucketLayoutInBinOpWarning is used if binary operators act on a
+// combination of two incompatible histograms.
+func NewIncompatibleBucketLayoutInBinOpWarning(operator string, pos posrange.PositionRange) error {
+	return annoErr{
+		PositionRange: pos,
+		Err:           fmt.Errorf("%w %s", IncompatibleBucketLayoutInBinOpWarning, operator),
+	}
+}


Test patch: