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

loki-mixin: use centralized configuration for dashboard matchers / selectors #4279

Merged
merged 7 commits into from
Oct 7, 2021

Conversation

kevinschoonover
Copy link
Contributor

@kevinschoonover kevinschoonover commented Sep 7, 2021

What this PR does / why we need it:
The dashboards have some boilerplate configuration copied at the top of each dashboard that means overwritting a selector, label, or otherwise must be done for each dashboard individually (at least thats what it looks like with my limited jsonnet experience). The PR adds 'job_names' and uses some of the unused functions in dashboard-utils.libsonnet to centralize the configuration and make it feel like cortex-mixin / tempo-mixin.

Dashboards updated:
- [ ] loki-logs.json - doesnt seem to be generated
- [ ] loki-operational.json - doesnt seem to be generated

  • loki-deletion.json
  • loki-chunks.json
  • loki-reads-resources.json
  • loki-reads.json
  • loki-retention.json
  • loki-writes-resource.json
  • loki-writes.json

@CLAassistant
Copy link

CLAassistant commented Sep 7, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay!
This looks pretty good. I'll admit I skimmed a bit, but I appreciate the refactoring. I've left a small question, but otherwise LGTM!

@kevinschoonover
Copy link
Contributor Author

@owen-d updated the PR to fixed your comment and rebased ontop of main. The merge conflicts were kinda garnly with #4401 so I hope I didn't miss something. Let me know if you have any other comments!

@owen-d
Copy link
Member

owen-d commented Oct 6, 2021

Just curious, are the dashboards performing as you expect?

@kevinschoonover
Copy link
Contributor Author

kevinschoonover commented Oct 7, 2021

When I was running them in isolation they were working (most likely because I was doing some overrides for my specific scenario). I just did a clean run of the dashboards and am seeing some missing recording rules, but this is likely because I'm using different selectors than stock (I'm using nomad and not k8s so namespace / datacenter instead of namespace / cluster).

I don't have a very complex deployment so I don't know how well my testing will translate but let me take a deep look at it over the weekend to make sure they're performing exactly as I expect. Will follow back up with you when this is done.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Thanks. This looks pretty good at a glance and we can clean it up afterwards if needed.

@owen-d owen-d merged commit 1a4be09 into grafana:main Oct 7, 2021
owen-d added a commit that referenced this pull request Oct 8, 2021
owen-d added a commit that referenced this pull request Oct 8, 2021
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