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

Baremetal dashboards #2657

Merged
merged 6 commits into from
Aug 25, 2022
Merged

Conversation

wilfriedroset
Copy link
Collaborator

What this PR does

Add support for baremetal deployments for the resources dashboards.
The default remains deployment_type: 'container', hence the mixin-compiled remains unchanged.
To test new dashboards update the configuration to deployment_type: 'baremetal' then build the mixins.

Which issue(s) this PR fixes or relates to

Relates #1642

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@wilfriedroset wilfriedroset force-pushed the baremetal-dashboards branch 2 times, most recently from 816b4a1 to 23c6c64 Compare August 12, 2022 06:26
@wilfriedroset wilfriedroset force-pushed the baremetal-dashboards branch 2 times, most recently from 7ef943a to d1be862 Compare August 19, 2022 05:27
@pracucci pracucci self-requested a review August 24, 2022 10:06
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

You did great! I left few minor comments.

@@ -78,6 +78,115 @@
gateway: helmCompatibleName('(gateway|cortex-gw|cortex-gw).*'),
},

deployment_type: 'container',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would call it kubernetes (instead of kubernetes) since some panels actually use K8S metrics. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed, I've made the changes accordingly.

|||,
},
baremetal: {
// Somes queries does not makes sens when running mimir on baremetal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Somes queries does not makes sens when running mimir on baremetal
// Somes queries does not makes sense when running mimir on baremetal

resourcesPanelLegend(first_legend)::
if $._config.deployment_type == 'container'
then [first_legend, 'limit', 'request']
// limit and request does not makes sens when running on baremetal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// limit and request does not makes sens when running on baremetal
// limit and request does not makes sense when running on baremetal

@@ -24,6 +24,10 @@ The following table shows the required label names and whether they can be custo

For rules and alerts to function properly, you must configure your Prometheus or Grafana Agent to scrape metrics from Grafana Mimir at an interval of `15s` or shorter.

## Deployment type

When running Grafana Mimir on baremetal, You can adjust the resulting dashboards via the `deployment_type` field in the mixin configuration.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
When running Grafana Mimir on baremetal, You can adjust the resulting dashboards via the `deployment_type` field in the mixin configuration.
By default, Grafana Mimir dashboards assume Mimir is deployed in containers orchestrated by Kubernetes.
If you're running Mimir on baremental, you should set the configuration field `deployment_type: 'baremetal'` and [re-compile the dashboards]({{< relref "../installing-dashboards-and-alerts.md" >}}).

CHANGELOG.md Outdated
@@ -20,6 +20,7 @@
### Mixin

* [ENHANCEMENT] Dashboards: added support to query-tee in front of ruler-query-frontend in the "Remote ruler reads" dashboard. #2761
* [ENHANCEMENT] Dashboards: Introduce support for baremetal deployment. #2657
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* [ENHANCEMENT] Dashboards: Introduce support for baremetal deployment. #2657
* [ENHANCEMENT] Dashboards: Introduce support for baremetal deployment, setting `deployment_type: 'baremetal'` in the mixin `_config`. #2657

This is the preliminary step to make room for supporting baremetal
deployment dashboards.

Signed-off-by: Wilfried Roset <wilfriedroset@users.noreply.github.com>
Queries and their arguments are in different files, hence using mapping
for string formating helps for the readability. One other benefit is
that it avoid to pass multiple time the same value or to pass exactly
the same number of attributes.

Signed-off-by: Wilfried Roset <wilfriedroset@users.noreply.github.com>
This is the preliminary step to make room for supporting baremetal
deployment dashboards.

Signed-off-by: Wilfried Roset <wilfriedroset@users.noreply.github.com>
The following commit introduce the support of baremetal deployment for
*resources* dashboards. It's done by completing the hash
`resources_panel_series` and `resources_panel_queries`. The promql
queries are based on node_exporter.

To accommodate panels where the number of query differ we rely on two
intermediate functions `resourcesPanelLegend` and
`resourcesPanelQueries` who are responsible for returning the proper
number of queries/legends.

Signed-off-by: Wilfried Roset <wilfriedroset@users.noreply.github.com>
As the dashboards supports both k8s and baremetal deployments,
let's use one single term to avoid confusion, hence instanceName.

Signed-off-by: Wilfried Roset <wilfriedroset@users.noreply.github.com>
Let's use a more meaning name for the deployment type. The metrics
relates to k8s metrics, as such having the value as kubernetes instead
of container makes more sense.

Signed-off-by: Wilfried Roset <wilfriedroset@users.noreply.github.com>
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Amazing work! 👏

@pracucci pracucci merged commit ac6b91c into grafana:main Aug 25, 2022
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.

None yet

2 participants