promql: Make it possible to add custom details in annotations and summarise multiple of the same annotation#15577
Conversation
|
Tagging @beorn7 and @krajorama because you reviewed the original PR |
56b8567 to
023c062
Compare
|
The only test failure seems flaky |
|
Huh, very weird test failure. (Not the race that we got hit by recently, see #15538. ) Rerunning the tests to see if it is flaky or consistent. And yeah, will review ASAP. |
beorn7
left a comment
There was a problem hiding this comment.
I think I understand now what you are up to, but it took me a while to understand.
First of all, let's try to implement this without involving the user of the annotations package. Ideally, we stick with the idea that annotations are just errors and handle all the other stuff behind the scenes (using type switches, type casts, or maybe just errors.As and errors.Is). This would also remove the AddRaw method, which is just cognitive ballast from the point of view of the user.
I have tried to sketch out the idea in comments. It might or might not work that way. Maybe you can try it out. If it doesn't work, I'll work on this more deeply and will come up with a more detailed (or different) suggestion. However, I'm about to leave for my Xmas vacations, so my next response will have to wait until the new (Gregorian) year. (Back in action 2025-01-07.) Sorry for that.
|
Hello from the bug-scrub! Do you think you will come back to this? My reaction is it seems like a lot of work for something of very low benefit to the user. |
|
Hi! Would like to come back to this eventually as I think it could be useful (open up the possibility of adding more detail in annotations in future), but it's a low priority as there's no immediate need, so I'll look at it again when there's more time. I agree it's not worth spending too much time on this for now (unless a good use case appears), so I'll timebox it, and will probably abandon it if it turns out to be too complicated / involve too many changes. |
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
This reverts commit 824f219. Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
This reverts commit ac9d848. Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
023c062 to
194838a
Compare
|
@beorn7 after quite some time, finally followed your suggestions and the code changes are much simpler now! |
|
Thanks. On my list. |
|
This is still on my review list. However, I might only get to it after PromCon. |
|
Sorry for further delay. I hope to get to this before this week ends. |
|
Apologies for further delay. This is still very close to the top of my list. |
beorn7
left a comment
There was a problem hiding this comment.
Nice. I just have a few documentation and style nits.
|
FYI: After holidays and sickness, I'm back in action. @zenador it seems there is very little left to get this past the finish line. Will you be able to work on this any time soon? |
Co-authored-by: Björn Rabenstein <github@rabenste.in> Signed-off-by: zenador <zenador@users.noreply.github.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
|
@beorn7 Finally updated :) Didn't realise there was so little, should have looked at it earlier |
|
Not sure why the DCO test has left us. I'll squash this anyway. |
…marise multiple of the same annotation (prometheus#15577) Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com> Signed-off-by: zenador <zenador@users.noreply.github.com> Co-authored-by: Björn Rabenstein <github@rabenste.in> Signed-off-by: Will Bollock <wbollock@linode.com>
…marise multiple of the same annotation (prometheus#15577) Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com> Signed-off-by: zenador <zenador@users.noreply.github.com> Co-authored-by: Björn Rabenstein <github@rabenste.in>
…marise multiple of the same annotation (prometheus#15577) Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com> Signed-off-by: zenador <zenador@users.noreply.github.com> Co-authored-by: Björn Rabenstein <github@rabenste.in>
…marise multiple of the same annotation (prometheus#15577) Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com> Signed-off-by: zenador <zenador@users.noreply.github.com> Co-authored-by: Björn Rabenstein <github@rabenste.in>
…marise multiple of the same annotation (prometheus#15577) Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com> Signed-off-by: zenador <zenador@users.noreply.github.com> Co-authored-by: Björn Rabenstein <github@rabenste.in>
…marise multiple of the same annotation (prometheus#15577) Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com> Signed-off-by: zenador <zenador@users.noreply.github.com> Co-authored-by: Björn Rabenstein <github@rabenste.in>
Previously, annotations contained all the info within the error string, and could not be combined, e.g. if you have these annotations, they could only show up as separate annotations:
So if you wanted to give more details, you would end up spamming the user with many of the same annotation type which is difficult to understand, and to avoid that you would have to simplify the warning to something like
PromQL warning: diff foundwith no detail.This PR makes it possible to combine them so you can be more detailed without spamming the user, e.g.
PromQL warning: 4 instances of diff of 3.0 to 4.5 from sources X, Y, Z, and this can be customised for each specific annotation as desired. Existing annotations which don't need custom treatment can be left as default. SeeTestAnnotations_AsStringsfor an example in practice.This can be used to improve annotations like
HistogramQuantileForcedMonotonicityInfo: #15578Supersedes #14339
This has no user-facing effects yet: