Skip to content

Split warnings and info annotations in API response #14327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jul 6, 2024
Merged

Split warnings and info annotations in API response #14327

merged 4 commits into from
Jul 6, 2024

Conversation

zenador
Copy link
Contributor

@zenador zenador commented Jun 20, 2024

Addresses #14135

@zenador
Copy link
Contributor Author

zenador commented Jun 20, 2024

@beorn7 PTAL

bboreham
bboreham previously approved these changes Jun 20, 2024
View reviewed changes
Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

This change lgtm.

The existing implementation by which errors.Is(err, PromQLInfo) is working seems a bit tricksy to me; better done via Go types.

@beorn7
Copy link
Member

beorn7 commented Jun 26, 2024

The existing implementation by which errors.Is(err, PromQLInfo) is working seems a bit tricksy to me; better done via Go types.

I guess this was my idea, seduced by the convenience of using fmt.Errorf with %w. It could be changed to a different approach, but obviously not as part of this PR.

beorn7
beorn7 requested changes Jun 26, 2024
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Thank you very much. Looks good in general. I just have a number of ideas about naming. See individual comments.

@beorn7
Copy link
Member

beorn7 commented Jun 26, 2024

The comment in lines 116 to 119 needs an update. (I cannot comment on unchanged lines directly, so I'm providing the link here.)

@beorn7 beorn7 mentioned this pull request Jun 26, 2024
Closed
@zenador zenador force-pushed the separate-info-anno branch from 53d1a03 to cbb5c42 Compare July 2, 2024 10:05
@zenador zenador marked this pull request as draft July 2, 2024 11:06
@zenador
Copy link
Contributor Author

zenador commented Jul 2, 2024

Making it a draft so we can wait for the frontend to update first before merging.

zenador added 2 commits July 3, 2024 17:55
@zenador
split warnings and info annotations in API response
6b0b2d4 
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador
update according to code review
92adad9 
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador zenador force-pushed the separate-info-anno branch from d6f46aa to 92adad9 Compare July 3, 2024 09:55
@zenador
Copy link
Contributor Author

zenador commented Jul 3, 2024

@beorn7 PTAL

beorn7
beorn7 previously approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. Thank you.

Do you think you could add a "minimal UI change" to this PR? Like rendering the infos in the same way as the warnings, just in a slightly less alarming color. That would be an MVP we could merge, and then we could ask the UI experts to make it nicer later, like including a button to hide infos or something.

@zenador
minimal UI change: show infos in different colour
82b2b64 
Signed-off-by: Jeanette Tan <jeanette.tan@grafana.com>
@zenador zenador marked this pull request as ready for review July 5, 2024 20:25
@zenador zenador requested review from juliusv and Nexucis as code owners July 5, 2024 20:25
@zenador
Copy link
Contributor Author

zenador commented Jul 5, 2024

@beorn7 Added the minimal UI change

juliusv
juliusv reviewed Jul 5, 2024
View reviewed changes
@juliusv
Copy link
Member

juliusv commented Jul 5, 2024

UI change looks good to me besides the comment on the color.

@zenador @juliusv
Update web/ui/react-app/src/pages/graph/Panel.tsx
36bc3fa 
Co-authored-by: Julius Volz <julius.volz@gmail.com>
Signed-off-by: zenador <zenador@users.noreply.github.com>
juliusv
juliusv approved these changes Jul 6, 2024
View reviewed changes
Copy link
Member

@juliusv juliusv left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@juliusv juliusv merged commit 480fefd into prometheus:main Jul 6, 2024
@beorn7 beorn7 mentioned this pull request Jul 6, 2024
Closed
@zenador zenador deleted the separate-info-anno branch July 9, 2024 03:21
@charleskorn charleskorn mentioned this pull request Jul 16, 2024
Merged
2 tasks
@rapphil rapphil mentioned this pull request Jul 25, 2024
Closed
@yeya24 yeya24 mentioned this pull request Aug 4, 2024
Open
@bboreham bboreham mentioned this pull request Sep 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusv juliusv juliusv approved these changes

@beorn7 beorn7 beorn7 left review comments

@bboreham bboreham bboreham left review comments

@Nexucis Nexucis Awaiting requested review from Nexucis Nexucis is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zenador @beorn7 @juliusv @bboreham