Before leaving the real review to @beorn7 the native histogram expert, a few quick observations / questions:

• Seems like the autocomplete for the new operator is still missing?
• Similar point for linting adjustments in the editor: an expression like foo </ bool 1 currently gives no linter error but fails with an error like bool modifier can only be used on comparison operators. An expression like 1 </ 1 gives the linter error comparisons between scalars must use BOOL modifier, but the bool modifier does not help / is not legal in this case. I guess there needs to be just more differentiation between the regular comparison operators and the native histogram trimming ones.
• Should the count and sum fields stay completely untouched as they currently are? I guess you can't really change sum in a good way, and thus you also keep count without removing the filtered elements from it? It's just a bit funky to get a derived histogram that has potentially no buckets at all anymore, but a big count and sum. Not sure what to do about that though (unless we also stored a sum per bucket or estimated it by doing count * (lower_bound + upper_bound)/2).
• If I try running non_histogram_metric </ 1, I get the error operator "</" not allowed for operations between Vectors. Maybe the error message should be more fitting here, saying something about the operator only working on native histograms and scalars?

Thanks for the feedback, @juliusv! I've made the following changes:

• ✅ Autocomplete support for the new operator (</, >/) has been added.

• ✅ The linter now properly distinguishes native histogram trimming operators from standard comparison operators:

  • foo </ bool 1 now triggers linting error:
    "bool modifier can only be used on comparison operators"
  • 1 </ 1 now triggers linting error:
    "operator "</" not allowed for Scalar operations"

• ✅ I’ve updated how the sum is handled for trimmed histograms:

  updatedCount += bucket.Count
  bucketMidpoint := (bucket.Lower + bucket.Upper) / 2
  updatedSum += bucket.Count * bucketMidpoint

  This way, sum is now roughly estimated per bucket, and count reflects only the buckets that remain after trimming.

• ✅ The error message for invalid usage like non_histogram_metric </ 1 now correctly states:
  "operator "</" is only supported for native histograms and scalars, not standard vector operations"

Let me know if you have any further suggestions!

@juliusv is it ready for review for @beorn7 ?

I'll have a look at this ASAP. Also, @NeerajGartia21 might help with the reviewing. He has worked a lot on histogram functions recently and has become one of the domain experts.

Thanks for the PR, @sujalshah-bit! I’ll review it in detail in 1–2 days. In the meantime, please add tests for the engine, lexer, and parser to ensure everything is working correctly. You can refer to this for adding tests to the PromQL engine.

1. There's a mismatch between the operator name and its behavior. In native histograms: Implement the </ and >/ operators to "trim" a histogram. #14651 (comment) , @beorn7 mentioned that histogram_metric </ 0.1 should remove observations greater than 0.1. This means it trims the right part of the histogram, so logically it should be called RTRIM. However, the PR description currently says that </ removes observations below the threshold, which contradicts both the discussion and the current implementation. Would be good to clarify this, but I’ll leave the final word to @beorn7.

While I can logically understand either variant, the second one feels way more intuitive to me when taking into account that traditional filter operators like > and < keep (and not drop) those series of a vector that match the filter condition. So I would expect histogram_metric </ 0.1 to keep only the observations less than 0.1 and inverting it would be confusing.

Thanks for the great discussions. I'll try to answer all the questions ASAP. (Still swamped with things after KubeCon.)

While I can logically understand either variant, the second one feels way more intuitive to me when taking into account that traditional filter operators like > and < keep (and not drop) those series of a vector that match the filter condition. So I would expect histogram_metric </ 0.1 to keep only the observations less than 0.1 and inverting it would be confusing.

Makes sense. We could also call them trimUpper and trimLower.

• </ as trimUpper (removes values > threshold)
• >/ as trimLower (removes values < threshold)

Might make things clearer.

While I can logically understand either variant, the second one feels way more intuitive to me when taking into account that traditional filter operators like > and < keep (and not drop) those series of a vector that match the filter condition. So I would expect histogram_metric </ 0.1 to keep only the observations less than 0.1 and inverting it would be confusing.

Makes sense. We could also call them trimUpper and trimLower.

• </ as trimUpper (removes values > threshold)
• >/ as trimLower (removes values < threshold)

Yeah, I think that kind of naming would make sense, it would mirror what e.g. Go does with TrimLeft() and TrimRight() (naming the part that is removed).

Just to confirm my understanding:

The intended behavior is that histogram_metric </ rhs keeps only values less than rhs (i.e., trims away values ≥ rhs), and >/ rhs keeps only values greater than rhs (i.e., trims away values ≤ rhs).
So the PR description needs to be corrected to match this behavior — is that right?

Just to confirm my understanding:

The intended behavior is that histogram_metric </ rhs keeps only values less than rhs (i.e., trims away values ≥ rhs), and >/ rhs keeps only values greater than rhs (i.e., trims away values ≤ rhs). So the PR description needs to be corrected to match this behavior — is that right?

Correct!

High level answers (I have not yet looked at the code):

WRT direction: I think the discussion above came to the right conclusion: histogram </ threshold means to filter out observation ≥ threshold and keep the observations < threshold (AKA TrimUpper), and vice versa for >/.

WRT interpolation: I believe we should do the interpolation. Linear interpolation for NHCB and in the zero bucket, and exponential interpolation for the standard exponential schemas. As @NeerajGartia21 said:

For reference, you can look at the interpolation logic in the histogram_quantile and histogram_fraction functions here.

WRT count and sum: Those need to be adjusted as well, as @sujalshah-bit already discussed. To add more detail to the provided formula:

• For count, we need to deduct the bucket count of entirely removed buckets. For the bucket into which we have interpolated, we need to deduct the count we have removed from that bucket as per the interpolation.
• For sum, we can indeed use the midpoint of each bucket for linear interpolation (i.e. for NHCB and the zero bucket). For the exponential bucket, we should use the geometric mean (cf. how it is done for histogram_stddev). For the bucket that we are only partially removing, we need to take the mean between the relevant remaining bucket boundary and the threshold. (I can flesh this out with an example if it isn't clear.)

@sujalshah-bit do you feel with the collected comments that you can go into the next iteration of working on the code?

My impression is that a code-level review only makes sense once you have worked in the high level notes above. If you need some code-level assistance, please let me know. (Note that @NeerajGartia21 already gave valuable code level comments.)

Also, of course, feel free to ask any questions you might still have to fully understand what we want to implement here.

(Side note: When mentioning the geometric mean for histogram_stddev above, I realized that this function should take the arithmetic mean for NHCB. I filed #16439 for it.)

Thanks a lot for the detailed discussion above — I’ve learned a lot, and will be updating the changes soon!

I'm trying to generate a customized bucket for native histograms for testing this feature, but it's not working as expected.

By expected I mean:

CustomValues []float64 // this is empty and,
FloatHistogram.UsesCustomBuckets() // this is returning false

Here's the code I'm using to define the histogram:

customBuckets = promauto.NewHistogram(prometheus.HistogramOpts{
	Name: "custom_bucketss",
	Help: "Histogram with manually defined custom buckets",
	Buckets: []float64{0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10},
	NativeHistogramBucketFactor: 1.1,
})
rng = rand.New(rand.NewPCG(uint64(time.Now().UnixNano()), 1))
customValue := rng.Float64() * 5.0
customBuckets.Observe(customValue)

Am I missing anything required to make it a proper NHCB (Native Histogram with Custom Buckets) for testing purposes?

@krajorama can you kindly review :)

Thanks, everyone. I'll give this a thorough review ASAP (hopefully tomorrow).

Should I go ahead and address this comment and this one in this PR, or should I work on those as a follow-up instead?

Either way, I’m happy to take care of it. Please let me know what works best.

Aside from those, I noticed only one other minor fix remaining (just have to push).

Sorry for further delay. I'm still trying to do a thorough review this week, but there are other deadlines looming…

@sujalshah-bit in the meantime, please feel free to push the minor fix you were talking about above.

Also, on first glance, it seems useful to incorporate @krajorama's ideas about treating ±Inf edge cases, especially because things might actually get more straight forward after it. @sujalshah-bit your call. If you think it's a good idea, do it, but if you think it's better to work on it in another PR, that's also fine. (Modulo concerns I might find after my thorough review.)

I was back in action for a few days, but now I'm going on vacations… @krajorama @sujalshah-bit if you find the time, please finish this without me. I'm taking a note that I'll check the logic of this in any case before we declare NH stable.

@krajorama I have made the changes you can have look.