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

Migrate thanos-mixin from kube-thanos #1760

Merged
merged 23 commits into from
Dec 13, 2019

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Nov 19, 2019

This PR moves thanos-mixin from kube-thanos.
Fixes #1180

thanos-mixin is a Jsonnet library to generate monitoring dashboards and Prometheus rules.
For further information on mixins, check out: https://docs.google.com/document/d/1A9xvzwqnFVSOZ5fD3blKODXfsat5fg6ZhnKu9LK3lB4/edit#heading=h.gt9r2h2gklj3

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

TO DO

  • Rename Jsonnet dir
  • Add README and improve documentation on how to generate examples
  • Remove vendor
  • Fix broken tests and linter issues

Changes

  • Adds thanos-mixin
  • Adds necessary Makefile rules to generate rules and dashboards.

Verification

Executed introduced Makefile rules.

@kakkoyun
Copy link
Member Author

cc @metalmatze @GiedriusS @bwplotka

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Tried out make examples and make rules-lint. It complains about:

Checking examples/alerts/rules.yaml
3 duplicate rules(s) found.                                                                                                                                                                                          
Metric: :api_range_query_duration_seconds:histogram_quantile                                                                                                                                                         
Label(s):                                                                                                                                                                                                            
        quantile: 0.99                                                                                                                                                                                               
Metric: :http_request_duration_seconds:histogram_quantile                                                                                                                                                            
Label(s):                                                                                                                                                                                                            
        quantile: 0.99
Metric: :thanos_objstore_bucket_operation_duration_seconds:histogram_quantile
Label(s):
        quantile: 0.99
Might cause inconsistency while recording expressions.

Should we fix them before merging or should we leave it for the future? If we would fix them now then we could also add make alerts-test to our CI ❤️

Also, I have checked that md5sum of alerts.yaml and rules.yaml match up with the ones from kube-thanos. I haven't checked the dashboards themselves but at least they get generated successfully.

examples/alerts/rules.yaml Show resolved Hide resolved
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Also, do we need to add jsonnet/vendor/* to our repositories given that jsonnetfile.lock.json exists?

examples/alerts/alerts.md Outdated Show resolved Hide resolved
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice and thanks for doing this!

I just looked on https://github.com/kubernetes-monitoring/kubernetes-mixin and I can see vendor is not pulled, maybe we should do the same for Thanos mixin? I would vote for that as we do not vendor Go deps as well (: WDYT?

Also not sure about jsonnet directory. We don't do go / jsonnet per language dirs, but rather we split functionally, do you think we can actually put it in mixin directory? I like the idea of generating examples, but we might want to add some docs how to update this from mixin (I assume you generated those from that jsonnet) (:

Plus I think README for mixin would be must have for this as it requires totally new tooling.

examples/alerts/rules.yaml Show resolved Hide resolved
@kakkoyun
Copy link
Member Author

Thanks a lot for the feedback, I'll address all issues.

I totally agree with vendor, I'll remove it. It's just a reflex to vendor dependencies. I should have followed conventions of the project.

I'm going to rename jsonnet dir to mixin, this sounds good to me.

And I'll also improve documentation.

@kakkoyun kakkoyun changed the title Migrate thanos-mixin from kube-thanos WIP: Migrate thanos-mixin from kube-thanos Nov 26, 2019
@kakkoyun kakkoyun changed the title WIP: Migrate thanos-mixin from kube-thanos Migrate thanos-mixin from kube-thanos Nov 28, 2019
@kakkoyun
Copy link
Member Author

@GiedriusS @bwplotka this PR is ready for another round :)

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice! I find it interesting that there is mixin/thanos-mixin dirs still, maybe mixin/thanos would make sense? But it's not super critical, happy to merge as it is - let me know.

mixin/thanos-mixin/config.libsonnet Outdated Show resolved Hide resolved
mixin/thanos-mixin/config.libsonnet Outdated Show resolved Hide resolved
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Yeah, so just a few minor things that were mentioned in-line and I think we can merge this 👍 the changes look good and rules/dashboards make sense. I haven't tried importing them but just glanced over the code. Thanks!

Copy link
Contributor

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

Forgot to send my review from a few days ago 🙈

mixin/thanos-mixin/config.libsonnet Outdated Show resolved Hide resolved
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Copy link
Member Author

Add fixes that introduced in thanos-io/kube-thanos#74.

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
bwplotka and others added 2 commits December 10, 2019 16:48
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Some suggestions around Makefile and LGTM!

mixin/thanos/README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
mixin/thanos/README.md Show resolved Hide resolved
mixin/thanos/README.md Show resolved Hide resolved
mixin/thanos/README.md Outdated Show resolved Hide resolved
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Copy link
Member Author

@bwplotka I've cleaned the Makefile up, PTAL.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM mod suggestions. TBH I am fine with merging this and changing those in next PR, this is quite big already.

Makefile Outdated Show resolved Hide resolved
examples/dashboards/dashboards.md Outdated Show resolved Hide resolved
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Copy link
Member Author

kakkoyun commented Dec 12, 2019

@GiedriusS @bwplotka @metalmatze I've fixed the pointed issues and renamed the components considering to #1871

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@bwplotka bwplotka merged commit cf535c8 into thanos-io:master Dec 13, 2019
@kakkoyun kakkoyun deleted the add_thanos_mixin branch December 13, 2019 14:18
@metalmatze
Copy link
Contributor

Nice! Thanks for the work on this everybody!

@kakkoyun kakkoyun mentioned this pull request Dec 13, 2019
2 tasks
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.

Remove gossip from Dashboards and benchmark tool
4 participants