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

WIP: production/promtail-mixin: Make dashboard queries configurable #1978

Closed
wants to merge 1 commit into from

Conversation

brancz
Copy link

@brancz brancz commented Apr 24, 2020

What this PR does / why we need it:

Note I'm not entirely happy with where configuration is, but this could potentially be a direction for resolving #1830, making the mixin more configurable and not quite force specific relabeling rules for them to work.

Which issue(s) this PR fixes:
Fixes #1830

Special notes for your reviewer:
Mainly looking for feedback at this point, before I put more work into doing this. One thing we could do is instead move the configuration to the root of the exported object, which would kind of force people to not use the global merge pattern that we sadly advocated for in the beginning of monitoring mixins (cc @sh0rez).
Checklist

  • Documentation added
  • Tests updated

@cyriltovena

@CLAassistant
Copy link

CLAassistant commented Apr 24, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cyriltovena
Copy link
Contributor

You mean you don't like the $_config at the root ?

@brancz
Copy link
Author

brancz commented Apr 24, 2020

I don't mind using config at the root, but I wouldn't want it to merged, but rather explicitly configured when used. Example:

local mixin1 = (import 'mixin1.libsonnet') {
  // configure mixin1
};

local mixin2 = (import 'mixin2.libsonnet') {
  // configure mixin2
};

{
  grafanaDashboards:: mixin1.grafanaDashboards + mixin2.grafanaDashboards,
}

Instead of "globally" merging mixins. But the location is something we can discuss in another forum as well (@sh0rez and a few others already started this a bit), let's rather focus on the configuration knobs themselves here.

@cyriltovena
Copy link
Contributor

cyriltovena commented Apr 24, 2020

I like the configuration knobs.

 showMultiCluster:: true,
 clusterLabel:: 'cluster',
matchers:: [utils.selector.eq('job', '$namespace/$name')],

We might want to make sure to show what is configurable since other variables are just for computation. It's a good start for sure.

@tomwilkie
Copy link
Contributor

I support this change! We recently did a similar thing for the Cortex mixin: grafana/cortex-jsonnet#43

We've stopped globally merging mixins a few weeks ago: https://github.com/grafana/jsonnet-libs/blob/master/prometheus-ksonnet/mixins.libsonnet#L3, but its kinda up to the user of the mixin.

I think we should still use the _config for individual mixin config.

@brancz
Copy link
Author

brancz commented Apr 28, 2020

I think we should still use the _config for individual mixin config.

Agreed. In particular something we've been experimenting with is specifying everything within _config as non-hidden fields, that way it's easy to load the configuration from json/yaml, which would make it easy to package mixin in an easier to consume format than jsonnet, and only the maintainers need to really know the ins and outs, while users can have a super simple experience.

clusterLabel:: 'cluster',
clusterMatchers:: if cfg.showMultiCluster then [utils.selector.eq(cfg.clusterLabel, '$cluster')] else [],

matchers:: [utils.selector.eq('job', '$namespace/$name')],
Copy link
Contributor

@chancez chancez May 21, 2020

Choose a reason for hiding this comment

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

What if our job label doesn't take this form? I've been using serviceMonitors with prometheus-operator and our jobLabels are unfortunately just the service name currently. I've been meaning to fix it but haven't quite figured out the best method to get job=$namespace/$name.

Copy link
Contributor

@chancez chancez May 21, 2020

Choose a reason for hiding this comment

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

I'm a jsonnet newbie, but In hindsight, I'm guessing I would just override matchers to do that. I'm kind wondering still if we could avoid using job labels entirely with dashboards since it's a label that seems fairly inconsistent, while pod, container and namespace should be fairly universal (once #2070 is fixed).

@stale
Copy link

stale bot commented Jun 20, 2020

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Jun 20, 2020
@brancz
Copy link
Author

brancz commented Jun 23, 2020

Still on my to do list

@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Jun 23, 2020
@cyriltovena cyriltovena added the keepalive An issue or PR that will be kept alive and never marked as stale. label Jun 23, 2020
@DaveOHenry
Copy link
Contributor

DaveOHenry commented Nov 29, 2021

Sorry, misread the title. Moved my comment to a separate issue.

@kavirajk
Copy link
Contributor

Closing this as no activities over year(s). Feel free to send new PR if anyone want's to revive the work

@kavirajk kavirajk closed this Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepalive An issue or PR that will be kept alive and never marked as stale. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

more configurable job label in mixin
7 participants