-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this 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.
Thanks a lot for the feedback, I'll address all issues. I totally agree with I'm going to rename And I'll also improve documentation. |
3273f7a
to
f0c85d4
Compare
@GiedriusS @bwplotka this PR is ready for another round :) |
There was a problem hiding this 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.
There was a problem hiding this 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!
There was a problem hiding this 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 🙈
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>
b4e9a37
to
4d95064
Compare
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>
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
There was a problem hiding this 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!
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@bwplotka I've cleaned the Makefile up, PTAL. |
There was a problem hiding this 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.
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>
@GiedriusS @bwplotka @metalmatze I've fixed the pointed issues and renamed the components considering to #1871 |
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Nice! Thanks for the work on this everybody! |
This PR moves
thanos-mixin
fromkube-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
TO DO
Changes
thanos-mixin
Makefile
rules to generate rules and dashboards.Verification
Executed introduced
Makefile
rules.