-
Notifications
You must be signed in to change notification settings - Fork 55
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
feature: Add text boxes and descriptions to reads and writes dashboards #324
feature: Add text boxes and descriptions to reads and writes dashboards #324
Conversation
Focussing on the reads and writes dashboards, added some info panels and hover-over descriptions for some of the panels. Some common code used by the compactor also received additional text content. New functions: - addRows - addRowsIf ...to add a list of rows to a dashboard. The `thanosMemcachedCache` function has had some of its query text sprawled out for easier reading and comparison with similar dashboard queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super sure about this change because of a couple of reasons:
- It makes some rows very verbose
- It adds 1 panel to some (already very crowded) rows, which will make it more difficult to render nicely on not-very-large screens
9394e02
to
c4db3e1
Compare
Perhaps we can block these changes with a $._config setting that is defaulted to false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use sentence case. For more information, refer to https://developers.google.com/style/capitalization#capitalization-in-titles-and-headings.
…ctor dashboard under tooltips, rather than making them text boxes.
@pracucci I've thinned out a lot of the original text, mostly removing the text boxes and tooltip descriptions in the top left of the panels, but leaving a "Description" text box at the top. I edited a few of the row names to try to be a bit more explicit. For example instead of To make the compactor dashboard a little easier to look at, I took the text that is already in the main branch and tucked it under the panel descriptions, eliminating the text boxes that are there today. With these updates, we shouldn't have any rows that are wider than 3 panels, except for the 4-panel wide object storage rows (which are unchanged from what they are today in the main branch). |
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
…github.com:grafana/cortex-jsonnet into darrenjaneczek/dashboard-descriptions-reads-writes
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
Co-authored-by: Ursula Kallio <73951760+osg-grafana@users.noreply.github.com>
…github.com:grafana/cortex-jsonnet into darrenjaneczek/dashboard-descriptions-reads-writes
- Most existing "per second" panel titles in `main` are written "/ sec", corrected recent commits to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this and addressing my feedback. I left few comments about tiny things to fix and then we can merge.
CHANGELOG.md
Outdated
@@ -12,6 +12,7 @@ | |||
* [BUGFIX] Fixed `CortexIngesterHasNotShippedBlocks` alert false positive in case an ingester instance had ingested samples in the past, then no traffic was received for a long period and then it started receiving samples again. #308 | |||
* [CHANGE] Dashboards: added overridable `job_labels` and `cluster_labels` to the configuration object as label lists to uniquely identify jobs and clusters in the metric names and group-by lists in dashboards. #319 | |||
* [CHANGE] Dashboards: `alert_aggregation_labels` has been removed from the configuration and overriding this value has been deprecated. Instead the labels are now defined by the `cluster_labels` list, and should be overridden accordingly through that list. #319 | |||
* [ENHANCEMENT] Added documentation text panels and descriptions to Reads and Writes dashboards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* [ENHANCEMENT] Added documentation text panels and descriptions to Reads and Writes dashboards. | |
* [ENHANCEMENT] Added documentation text panels and descriptions to Reads and Writes dashboards. #324 |
getObjectStoreRows(title, component):: [ | ||
($.row(title) { height: '25px' }) | ||
.addPanel( | ||
$.textPanel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this? Looks like what it describes is pretty trivial. You can easily notice it's the latency and that it displays avg, median and 99th percentile.
CHANGELOG.md
Outdated
@@ -12,6 +12,7 @@ | |||
* [BUGFIX] Fixed `CortexIngesterHasNotShippedBlocks` alert false positive in case an ingester instance had ingested samples in the past, then no traffic was received for a long period and then it started receiving samples again. #308 | |||
* [CHANGE] Dashboards: added overridable `job_labels` and `cluster_labels` to the configuration object as label lists to uniquely identify jobs and clusters in the metric names and group-by lists in dashboards. #319 | |||
* [CHANGE] Dashboards: `alert_aggregation_labels` has been removed from the configuration and overriding this value has been deprecated. Instead the labels are now defined by the `cluster_labels` list, and should be overridden accordingly through that list. #319 | |||
* [ENHANCEMENT] Added documentation text panels and descriptions to reads and writes dashboards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] We're used to link the related PR.
* [ENHANCEMENT] Added documentation text panels and descriptions to reads and writes dashboards. | |
* [ENHANCEMENT] Added documentation text panels and descriptions to reads and writes dashboards. #324 |
$.panelDescription( | ||
'Compacted blocks / sec', | ||
||| | ||
Display the amount of time that it’s taken to generate a single compacted block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description looks wrong here.
Display the amount of time that it’s taken to generate a single compacted block. | |
Rate of blocks that are generated as a result of a compaction operation. |
$.panelDescription( | ||
'Per-block compaction duration', | ||
||| | ||
Rate of blocks that are generated as a result of a compaction operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description looks wrong here.
Rate of blocks that are generated as a result of a compaction operation. | |
Display the amount of time that it’s taken to generate a single compacted block. |
$.panel('QPS') + | ||
$.queryPanel('sum by(operation) (rate(thanos_memcached_operations_total{%s,component="%s",name="%s"}[$__rate_interval]))' % [$.jobMatcher(jobName), component, cacheName], '{{operation}}') + | ||
$.panel('Requests per second') + | ||
$.queryPanel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers: just reformatted.
jobMatcher: $.jobMatcher(jobName), | ||
component: component, | ||
cacheName: cacheName, | ||
cacheNameReadable: std.strReplace(cacheName, '-', ' '), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused. I think can be removed.
) | ||
.addPanel( | ||
$.panel('Latency (getmulti)') + | ||
$.latencyPanel('thanos_memcached_operation_duration_seconds', '{%s,operation="getmulti",component="%s",name="%s"}' % [$.jobMatcher(jobName), component, cacheName]) | ||
$.latencyPanel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers: just reformatted.
cacheName, | ||
], 'items') + | ||
{ yaxes: $.yaxes('percentunit') }, | ||
$.queryPanel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers: just reformatted.
), | ||
|
||
filterNodeDiskContainer(containerName):: | ||
||| | ||
ignoring(%s) group_right() (label_replace(count by(%s, %s, device) (container_fs_writes_bytes_total{%s,container="%s",device!~".*sda.*"}), "device", "$1", "device", "/dev/(.*)") * 0) | ||
||| % [$._config.per_instance_label, $._config.per_node_label, $._config.per_instance_label, $.namespaceMatcher(), containerName], | ||
ignoring(%s) group_right() ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers: just reformatted.
@@ -5,20 +5,42 @@ local utils = import 'mixin-utils/utils.libsonnet'; | |||
($.dashboard('Cortex / Writes') + { uid: '0156f6d15aa234d452a33a4f13c838e3' }) | |||
.addClusterSelectorTemplates() | |||
.addRow( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new row takes quite a lot of vertical space.
To make everyone happy, could we add a config option to enable/disable the dashboard description and conditionally add this row only when enabled? I'm fine to keep it enabled by default but would allow us to disable it in our infra.
Same comment applies to the read dashboard. A single config option shared between the 2 dashboards looks fine.
'Tenants compaction progress', | ||
||| | ||
In a multi-tenant cluster, display the progress of tenants that are compacted while compaction is running. | ||
Reset to `0` after the compaction run is completed for all tenants in the shard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fix the 0
by wrapping it in <tt/>
instead.
I'll ask about the dark heading font.
Thanks for the heads up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks! Keep in mind I've commented here, but this may apply to other placesu/descriptions too (eg. the heading color applies to any description).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
…czek/dashboard-descriptions-reads-writes feature: Add text boxes and descriptions to reads and writes dashboards
What this PR does:
Focussing on the reads and writes dashboards,
added some info panels and hover-over descriptions
for some of the panels.
Some common code used by the compactor also
received additional text content.
New functions:
...to add a list of rows to a dashboard.
The
thanosMemcachedCache
function has had some of its query textsprawled out for easier reading and comparison with similar dashboard
queries.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]