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

feat(mixin): Add pod prefix config option (non-SSD) #14624

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mtweten
Copy link

@mtweten mtweten commented Oct 25, 2024

What this PR does / why we need it:
When deploying loki in distributed mode using the combined "production" loki helm chart (https://github.com/grafana/loki/tree/main/production/helm/loki), pod names will be prefixed depending on the helm release name. Typically this prefix would be "loki-" (e.g. loki-distributor). Currently, the mixin dashboards don't account for this and so some panels don't work correctly.

This PR introduces a pod_prefix_matcher at the top level of config which is used to prefix pod names in pod matchers. It defaults to an empty string for passivity. Typically, this will probably be set to "loki-", but can be different based on helm release names used.

I'm not sure if this is the best name for this config option, but it mirrors the existing _config.ssd.pod_prefix_matcher config item.

One thing to note - the existing _config.ssd.pod_prefix_matcher is also used in job matchers, but I didn't update any job matchers with the new _config.pod_prefix_matcher. However, I would be open to making that change as well, which I believe would provide an alternative solution to #13631 (which currently requires adding some relabeling to loki's servicemonitor to resolve).

Which issue(s) this PR fixes:
This is sort of tangentially related to #13631 - but that can be fixed with some relabeling rules on the loki servicemonitor so I didn't add the prefix to the job selectors.

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@mtweten mtweten requested a review from a team as a code owner October 25, 2024 17:28
@CLAassistant
Copy link

CLAassistant commented Oct 25, 2024

CLA assistant check
All committers have signed the CLA.

@@ -2,7 +2,7 @@ local g = import 'grafana-builder/grafana.libsonnet';
local utils = import 'mixin-utils/utils.libsonnet';

(import 'dashboard-utils.libsonnet') {
local compactor_matcher = 'pod=~"(compactor|%s-backend.*|loki-single-binary)"' % $._config.ssd.pod_prefix_matcher,
local compactor_matcher = 'pod=~"(%scompactor.*|%s-backend.*|loki-single-binary)"' % [$._config.pod_prefix_matcher, $._config.ssd.pod_prefix_matcher],
Copy link
Author

@mtweten mtweten Oct 25, 2024

Choose a reason for hiding this comment

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

I also added a missing .* in this selector to the end of compactor, which is why the generated dashboards above have changed. I believe this was a defect - see the equivalent matcher in loki-retention.libsonnet which already has the .*.

then 'pod=~"(compactor.*|%s-backend.*|loki-single-binary)"' % $._config.ssd.pod_prefix_matcher

@chaudum
Copy link
Contributor

chaudum commented Nov 27, 2024

Hey @mtweten thanks for your contribution! Looks good to me. Could you please update your branch and resolve the conflicts?

@mtweten
Copy link
Author

mtweten commented Nov 27, 2024

@chaudum Thanks for the review! I resolved the conflicts - it was mainly with the ingester related lines where a partition-ingester matcher had also been added. I didn't add the prefix to the partition-ingester matcher - honestly I don't know if it should also be added there or not. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants