Add min/max/count info to annotations#14339
Conversation
|
@krajorama as discussed previously. this is just a starting point, we can modify the info to whatever would be helpful in escalations |
| // are likely artifacts of floating point precision issues) have been | ||
| // ignored. | ||
| func bucketQuantile(q float64, buckets buckets) (float64, bool, bool) { | ||
| func bucketQuantile(q float64, buckets buckets) (float64, float64, float64, float64, bool, bool) { |
There was a problem hiding this comment.
Please update the comment above the function it's outdated now.
| // are likely artifacts of floating point precision issues) have been | ||
| // ignored. | ||
| func bucketQuantile(q float64, buckets buckets) (float64, bool, bool) { | ||
| func bucketQuantile(q float64, buckets buckets) (float64, float64, float64, float64, bool, bool) { |
There was a problem hiding this comment.
Also I'd named the return values for clarity in the signature, probably makes the code little but simpler as well
| forcedMonotonicMaxBucket = buckets[i].upperBound | ||
| } | ||
| if diff := prev - curr; diff > forcedMonotonicMaxDiff { | ||
| forcedMonotonicMaxDiff = diff |
There was a problem hiding this comment.
I think showing non consequtive le values in the info can be confusing, so I'd store here the upperBounds of prev and curr and return that. forcedMonotonicMinBucket -> forcedMonotonicLower, forcedMonotonicMaxBucket -> forcedMonotonicUpper
| PositionRange posrange.PositionRange | ||
| Err error | ||
| Query string | ||
| Min []float64 |
There was a problem hiding this comment.
We've discussed this IRL, I think it will be tricky to come up with a good generic data structure that handles all future use cases for annotations. What you need is an interface that has Merge function. The implementation would do a type assertion and return error on failure and do the merge when the type assert is successful. And Annotations Add and Merge could use the interface. We wouldn't expect error from merge, since the key corresponds to the actual type I guess.
Come to think of it, you could also add a Level function to the interface that could tell you if the annotation is an Info or Warning .
There was a problem hiding this comment.
The info and warning distinction can be made with errors.Is (as we modeled annotations as Go errors). (#14327 shows it in action, but you see concerns in the review that it might be too tricksy.)
However, given that all annotations are just treated as Go errors, we can change the implementation for just some of them. Maybe it would be cleaner to let NewHistogramQuantileForcedMonotonicityInfo return an implementation specific to that type of annotation and don't even try to be generic.
beorn7
left a comment
There was a problem hiding this comment.
Thanks for doing this, @zenador . And thanks for reviewing, @krajorama . Just a few comments from the sideline.
| PositionRange posrange.PositionRange | ||
| Err error | ||
| Query string | ||
| Min []float64 |
There was a problem hiding this comment.
The info and warning distinction can be made with errors.Is (as we modeled annotations as Go errors). (#14327 shows it in action, but you see concerns in the review that it might be too tricksy.)
However, given that all annotations are just treated as Go errors, we can change the implementation for just some of them. Maybe it would be cleaner to let NewHistogramQuantileForcedMonotonicityInfo return an implementation specific to that type of annotation and don't even try to be generic.
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
|
@zenador do you still plan to work on this? |
|
Yes it's still on my todo list, but lower priority. |
|
Reworked and split into separate PRs, one for modifying annotations to add this capability and one to improve |
Allows combining info across multiple instances of the same annotation to create a summary, and implements this for HistogramQuantileForcedMonotonicityInfo to give the end user more information about it to see how widespread the problem is (helpful to see if it is likely to have affected the results and is worth worrying about, and to troubleshoot). Example from manual testing with #13725 reverted locally to force this to popup:
