Skip to content

Conversation

@fpetkovski
Copy link
Collaborator

This commit tries to simplify warnings by returning them from scalar accumulators instead of requiring a context to be passed in. It tries to return string constants wherever possible to avoid extra allocations.

Vector accumulators still need some work since they can raise multiple warnings.

@MichaHoffmann
Copy link
Contributor

I think we need the warnings type and the (warning, error) return signature because "histogram.Add" or any other could return an unkown error.
I think further that we could just take that unkonwn error and map it to "BadHistogramOperationWarning" and simplify the signature to only return an error that we always add to annotations - this would simplify everything alot.. because really we know all errors that histogram.Add can return and they really all map to known warnings already.

if errors.Is(err, histogram.ErrHistogramsIncompatibleSchema) {
warnings.AddToContext(annotations.MixedExponentialCustomHistogramsWarning, ctx)
return nil
return annotations.MixedExponentialCustomHistogramsWarning, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why dont we just retrn the error and have a "ErrorToWarning" method in some package that maps some known errors to warnings - and then we add the error that was returned to annotations in the caller. I think this would make the flow much simpler while at the same time pretty much all errors here just ocnvert to warnings and a dropped series for the timestamp anyway.
No error that t.Add could return should justifiably stop query execution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that going from warning to error and back to warning can be error prone and cumbersome to manage. We can just take the warning as it is and add it to the context. Computations should not raise errors anyway, those can only come from the network.

Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
@MichaHoffmann MichaHoffmann self-requested a review October 16, 2025 09:40
@MichaHoffmann MichaHoffmann merged commit 6a2ae45 into thanos-io:main Oct 17, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants