Skip to content

[loki] Generate Dashboards from loki-mixin#193

Open
jkroepke wants to merge 4 commits intografana-community:mainfrom
jkroepke:jsonnet
Open

[loki] Generate Dashboards from loki-mixin#193
jkroepke wants to merge 4 commits intografana-community:mainfrom
jkroepke:jsonnet

Conversation

@jkroepke
Copy link
Copy Markdown
Member

What this PR does / why we need it

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [grafana])

Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
@jkroepke jkroepke marked this pull request as ready for review March 28, 2026 06:16
QuentinBisson
QuentinBisson previously approved these changes Mar 28, 2026
Copy link
Copy Markdown

@QuentinBisson QuentinBisson left a comment

Choose a reason for hiding this comment

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

At a first glance LGTM

# -- If enabled, create PrometheusRule resources with Loki recording rules
enabled: false

configs:
Copy link
Copy Markdown
Member

@TheRealNoob TheRealNoob Mar 28, 2026

Choose a reason for hiding this comment

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

One of the things I remember is that this logic on configs.<alert_name>.lookbackPeriod and threshold won't scale to all alerts. It works for simple alerts, but won't scale if loki-mixin decides to add a more advanced query. For example

  foo[5m] > 0
and
  bar[10m] != 2

So I was going to look at removing these. And furthermore I was going to restructure .config to match KPS's implementation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah it looks like you've dropped support for the config block already

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I do not touch alerts here, only recording rules.

labels:
grafana_dashboard: "1"
multiCluster:
# -- Enable multi-cluster support for the dashboards.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this cluster term used to define kubernetes clusters or loki clusters? I assume it's loki cluster meaning support for multiple loki helm chart releases. it's an overused term and I wish mimir/loki had chosen something else.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know how feasiable it is, but I recall wishing in the past that we could change the term. Though I suspect this change would have to start at the loki-mixin.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Kubernetes cluster.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

at least in the current implementation of the dashboards in the chart, the cluster drop-down variable uses the cluster variable. which is what's defined in the serviceMonitor. So implementing this will probably cause both k8s cluster names and loki cluster names to be returned in the drop-down. so perhaps we want to hold-off on implementing this until the overlap is addressed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would say to address the overlap in this PR, but like I said the change will have to start in the loki-mixin. Unless you want to substitute it from the dashboard configmaps via python.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any thoughts on the label to use instead? I was thinking lokiRelease or releaseName? perhaps the second that way we can use the same variable across mimir/loki/tempo

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm willing to open the PR against loki-mixin. I've recently learned jsonnet. At least enough to do this PR. I'll make the variable configurable, as I suspect they'll ask for backwards compatibility. Depending on what the default is set to.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have no idea. Generally cluster label is used for Kubernetes Cluster. Loki should use loki_cluster.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Loki_cluster makes sense here to not overlap with the regular k8s cluster label. But I recall this can already be changed using a custom jsonnet config file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh that's good, didn't know that. I however lean towards a single label that can be re-used across tempo/mimir/loki.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would appreciate some time to test it so I can visualize the changes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure, community is invited as well. cc @marcofranssen

Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
@TheRealNoob TheRealNoob self-assigned this Apr 3, 2026
jkleinlercher added a commit to suxess-it/kubriX that referenced this pull request Apr 7, 2026
jkleinlercher added a commit to suxess-it/kubriX that referenced this pull request Apr 7, 2026
* feat(deps): update helm release k8s-monitoring to v4

BREAKING CHANGE: k8s-monitoring: adjust your custom values according to https://github.com/grafana/k8s-monitoring-helm/tree/main/charts/k8s-monitoring#version-40 (or contact kubrix support)

* Refactor k8s-monitoring configuration and remove alloy metrics

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

* Update values-kubrix-default.yaml

set prefix until grafana-community/helm-charts#193 is implemented

* Update values-kubrix-default.yaml

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Johannes Kleinlercher <johannes.kleinlercher@suxess-it.com>
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.

[loki] Dashboards for Distributed mode

3 participants