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.

Sign up for GitHub

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

Jump to bottom

Make the cortex-mixin work on non-k8s deployments. #43

Merged
merged 10 commits into from
Apr 21, 2020
Merged

Make the cortex-mixin work on non-k8s deployments. #43

merged 10 commits into from
Apr 21, 2020

Conversation

tomwilkie
Copy link
Contributor

@tomwilkie tomwilkie commented Apr 19, 2020 •
edited
Loading

But first, a bunch of refactoring to tidy the dashboards up:

  • Put all the mixin config in one place.
  • Make dashboardWithTagsAndLinks just an override on the dashboard constructor.
  • Factor out the helper functions.
  • Move the dashboards to separate files and namespace them off so they only have access to the config.
  • Add a AddRowIf helper to massively rationalise the inclusion / exclusion of rows.
  • Alter how we conditionally include dashboards themselves.
  • Use set-style selector for bigtable/cassandra/dynamodb.
  • Make the mixin by default not need any config, and generate dashboards for all possible permutations of tsdb, chunks, bigtable, dynamodb, cassandra, gcs, s3 etc.

@tomwilkie
Small refactors:
62c70ce 
- Put all the mixin config in one place.
- Make dashboardWithTagsAndLinks just an override on the dashboard constructor.
- Factor out the helper functions.

Signed-off-by: Tom Wilkie <tom@grafana.com>
@tomwilkie
More refactoring:
78349a8 
- Move the dashboards to separate files and namespace them off so they only have access to the config.
- Add a AddRowIf helper to massively rationalise the inclusion / exclusion of rates.
- Alter how we conditionally include dashboards themselves.

Should be a no-op change to the dashboards themselve.

Signed-off-by: Tom Wilkie <tom@grafana.com>
@tomwilkie
Use set-style selector for bigtable/cassandra/dynamodb.
d299d64 
Signed-off-by: Tom Wilkie <tom@grafana.com>
@tomwilkie
Make selectors in PromQL queries on the writes dashboard support sing…
2f71235 
…le process mode.

Signed-off-by: Tom Wilkie <tom@grafana.com>
@tomwilkie
Get all the panels on the write dashboard working with the single bin…
ef468e3 
…ary.

Signed-off-by: Tom Wilkie <tom@grafana.com>
@tomwilkie
Get read dashboard working for single process.
c706d9a 
Signed-off-by: Tom Wilkie <tom@grafana.com>
jtlisi
jtlisi reviewed Apr 19, 2020
View reviewed changes
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

I took a quick look and had a few questions. I noticed some dashboards and the recording rules still have cluster hardcoded but I assume that will come later.

I really like the new style of using functions to generate these things.

cortex-mixin/config.libsonnet Outdated Show resolved Hide resolved
cortex-mixin/config.libsonnet Outdated Show resolved Hide resolved
cortex-mixin/dashboards/dashboard-utils.libsonnet

// The ,ixin allow specialism of the job selector depending on if its a single binary
// deployment or a namespaced one.
jobMatcher(job)::
Copy link
Contributor

Choose a reason for hiding this comment

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

How are you going differentiate the metrics from each module using a Job? Is it going to be magic in the scrape config or are we going to allow cortex to run with component specific registries upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is actually much of a problem - redundant metrics exported by jobs (ie ingester metrics from the distributor) don't influence the final results (ie qps or latency). For almost all cases, we can target specific eg methods to get the qps/latency of a specific component. In some cases (kv stores) the metrics aren't broken out like that yet[1], but for eg caches they are. So I think we'll be okay.

Do you have a specific case in mind?

[1]cortexproject/cortex#2484

@tomwilkie @jtlisi
Apply suggestions from code review
a3c106c 
Co-Authored-By: Jacob Lisi <jacob.t.lisi@gmail.com>
@tomwilkie
Copy link
Contributor Author

Thanks for the feedback Jacob!

I noticed some dashboards and the recording rules still have cluster hardcoded but I assume that will come later.

I think we can get away with still having the cluster label in recording rules, even when the underlying metric doesn't have a cluster label. Its basically just ignored, and I prefer having consistent recording rules across deployments. We might even have multiple clusters of single process Cortex, and re-introduce the cluster label...

@tomwilkie
Update chunks and queries dashboards.
848899a 
Signed-off-by: Tom Wilkie <tom@grafana.com>
@tomwilkie tomwilkie marked this pull request as ready for review April 19, 2020 20:13
pstibrany
pstibrany approved these changes Apr 20, 2020
View reviewed changes
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Result looks good to me, new organization makes lot of sense. I don't appreciate monster PR – these changes would be easy to split into smaller PRs.

Also, in my Grafana I have datasources pointing to single-binary Cortex and multi-service Cortex as well. Do you think it would be possible to handle such deployments by your dashboards somehow?

cortex-mixin/dashboard-utils.libsonnet Show resolved Hide resolved
@tomwilkie
Only add the links if addClusterSelectorTemplates is called.
bb0b966 
Signed-off-by: Tom Wilkie <tom@grafana.com>
@tomwilkie
Only tag dashboards with cluster selectors.
4539b86 
Signed-off-by: Tom Wilkie <tom@grafana.com>
@tomwilkie tomwilkie merged commit b6263cd into master Apr 21, 2020
@tomwilkie tomwilkie deleted the optional-k8s branch April 21, 2020 11:47
@tomwilkie tomwilkie mentioned this pull request Apr 28, 2020
Closed
2 tasks
@pracucci pracucci mentioned this pull request May 6, 2020
Merged
simonswine pushed a commit to grafana/mimir that referenced this pull request Oct 18, 2021
@tomwilkie
Merge pull request grafana/cortex-jsonnet#43 from grafana/optional-k8s
ffa590a 
Make the cortex-mixin work on non-k8s deployments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtlisi jtlisi jtlisi left review comments

@pstibrany pstibrany pstibrany approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomwilkie @pstibrany @jtlisi