Skip to content
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.

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

Adding descriptions to prombench dashboard #428

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

imrajdas
Copy link
Contributor

@imrajdas imrajdas commented Aug 21, 2020

@geekodour
Copy link
Member

geekodour commented Aug 22, 2020

Can you split the indentation changes/other changes into a separate PR from the PR adding the description, like discussed offline?

It'll be easier to see the changes that way :)

Copy link
Member

@geekodour geekodour left a comment

Choose a reason for hiding this comment

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

nice :) Could you please link the google doc in the PR description which has the Panel title along with the description in tabular format? It seems that GitHub diff cuts the panel title and it becomes very hard to see where this description actually belongs to.

I've added some general dashboard related comments.

@@ -2349,6 +2350,7 @@ data:
"dashLength": 10,
"dashes": false,
"datasource": "prometheus-meta",
"description": "This dashboard indicates how many time series each instance holds",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"description": "This dashboard indicates how many time series each instance holds",
"description": "This graph indicates how many time series each instance holds",

This is a little opinionated but maybe we can completely omit the words "This dashboard"/"This graph" because one will be able to see the description by hovering over the i tag on that particular grafana panel only. In some cases maybe having this will make sense :)

Same for the other cases.

@@ -2896,6 +2903,7 @@ data:
"dashLength": 10,
"dashes": false,
"datasource": "prometheus-meta",
"description": "This dashboard shows two metrics\n- prometheus_tsdb_head_min_time: Minimum timestamp of the head block\n- prometheus_tsdb_head_max_time: Maximum timestamp of the head block\n",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's the best idea to simply have the metric names in the description. :) One could look that up in the graph editor anyway.

A more friendly description (based on your current description) would be something like "Plots of minimum and maximum timestamp of the head block in reference to current time", you could go on further to explain what the timestamp of the head block may signify.

@@ -3381,6 +3393,7 @@ data:
"dashLength": 10,
"dashes": false,
"datasource": "prometheus-meta",
"description": "This dashboard shows the following metrics.\n- Total WAL corruptions\n- Total TSDB reload failures\n- Head series not found\n- Total Compactions failures\n- Total retentions cutoffs failures",
Copy link
Member

Choose a reason for hiding this comment

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

The problems can already be seen in the graph legends, so we might just want to keep it short something like "Various tsdb related checks", or in this case, since the title is pretty much self explanatory, we may not even need a description here.

Or the other idea could be to mention cause and effects of these tsdb issues in the description.

Interested in hearing what others have to say about this. :)

@imrajdas
Copy link
Contributor Author

@bboreham
Copy link
Member

@geekodour do you think we could merge this?

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.

Approving on the basis (a) previous comments seem to have been addressed and (b) the added text seems reasonable to me.

Thanks!

@bboreham bboreham merged commit 475461d into prometheus:master Apr 2, 2024
Copy link

github-actions bot commented Apr 2, 2024

Grafana dashboard updated successfully.

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.

3 participants