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

mixin: Introduce flexible multi-cluster/namespace mode for alerts and dashboards #3856

Merged
merged 18 commits into from
Mar 19, 2021
Merged

mixin: Introduce flexible multi-cluster/namespace mode for alerts and dashboards #3856

merged 18 commits into from
Mar 19, 2021

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Mar 1, 2021 •
edited
Loading

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

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

Changes

  • Add namespace to the alerting rules and annotations

Verification

  • make examples
  • make example-rules-lint

Merge after: #3854
Fixes: #3856

@kakkoyun kakkoyun changed the title mixin: Add namespace to the alerts WIP: mixin: Add namespace to the alerts Mar 1, 2021
bwplotka
bwplotka reviewed Mar 1, 2021
View reviewed changes
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.

I would love this PR, but unfortunately, it also has those pod selector changes, which I don't understand 🤔

@kakkoyun kakkoyun marked this pull request as draft March 2, 2021 15:37
@kakkoyun kakkoyun changed the title WIP: mixin: Add namespace to the alerts mixin: Add namespace to the alerts Mar 2, 2021
@kakkoyun kakkoyun marked this pull request as ready for review March 2, 2021 15:49
@kakkoyun kakkoyun force-pushed the mixin_add_alert_namespaces branch 2 times, most recently from ad13ea2 to 03e314d Compare March 4, 2021 17:23
@kakkoyun kakkoyun requested a review from bwplotka March 4, 2021 19:04
@kakkoyun
Copy link
Member Author

kakkoyun commented Mar 5, 2021

Friendly ping @metalmatze @bwplotka

@kakkoyun kakkoyun requested a review from squat March 5, 2021 10:06
@brancz
Copy link
Member

brancz commented Mar 5, 2021

Didn't we just work on getting rid of kubernetes specific things?

@kakkoyun
Copy link
Member Author

kakkoyun commented Mar 5, 2021

Yes, definitely @brancz. I'll be introducing further changes to provide a more abstract solution for this #3867. It'll require more consideration. This would solve our problems in the short-term.

@kakkoyun
Copy link
Member Author

kakkoyun commented Mar 5, 2021

Ok, I'm convinced that we need to generalize this. Marking it WIP and jumping to fix these.

@kakkoyun kakkoyun marked this pull request as draft March 5, 2021 10:41
@kakkoyun
Add namespaces to alerts
c3033e6 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Fix redundant messages
21b9df4 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Generalize adding hierarcies to dashboards
44b7701 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun changed the title mixin: Add namespace to the alerts mixin: Introduce flexible multi-cluster/namespace mode for alerts and dashboards Mar 5, 2021
@kakkoyun
Generalize adding hierarcies to alert
cc96899 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun marked this pull request as ready for review March 5, 2021 18:14
@kakkoyun kakkoyun requested a review from brancz March 5, 2021 18:14
@kakkoyun
Update changelog
1c2acb4 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Copy link
Member Author

kakkoyun commented Mar 9, 2021

Friendly ping @bwplotka @metalmatze @brancz. Could you please take one last look at it?

bwplotka
bwplotka requested changes Mar 9, 2021
View reviewed changes
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.

I wonder if we should not have by (job, namespace) in most of those places to ensure we are not reducing all to one value. If we have multiple jobs in different namespaces we would count those as one series with this PR, no? 🤔

examples/dashboards/bucket_replicate.json Outdated Show resolved Hide resolved
examples/dashboards/bucket_replicate.json Outdated Show resolved Hide resolved
examples/dashboards/overview.json Outdated Show resolved Hide resolved
@kakkoyun
Copy link
Member Author

@bwplotka As stated in the issue(#3867) we try to decouple from any platforms by removing specific notions from the mixin. Thus I removed namespace from all the examples. However, we provide a flexible way to add any hierarchy to the alerts and dashboards through configuration. So it's okay to remove namespace. I have already added the necessary changelog entry.

bwplotka
bwplotka requested changes Mar 10, 2021
View reviewed changes
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.

I understand that, however, the problem is that we are summing namespaces and potentially cluster (whatever users have) into one value.

examples/dashboards/bucket_replicate.json Outdated Show resolved Hide resolved
mixin/dashboards/compact.libsonnet Outdated Show resolved Hide resolved
@kakkoyun
Copy link
Member Author

Thanks for pointing out. I'll make sure all the queries are modified with this way.

@kakkoyun kakkoyun marked this pull request as draft March 10, 2021 12:54
@kakkoyun
Add missing aggregators
e8bc898 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun marked this pull request as ready for review March 12, 2021 09:54
@kakkoyun
Copy link
Member Author

@bwplotka It's ready for another round. I've added missing aggregators.

@kakkoyun
Dashboard overhaul
f1b39ed 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Fix templating
ed53737 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Fix formatting issues
f235a11 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Refactor local variable
9b5c270 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Fix faulty aggregator argument usage
bc0a9d1 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Fix metric name issue
bb88157 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Add grpc_method to dashboards
7d7b3c3 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Add missing instance aggregation
d14d37f 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Fix overview dashboards
cf65979 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Strip white spaces
76ecdd7 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun
Fix go resouse dashboards
895c25e 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
bwplotka
bwplotka reviewed Mar 19, 2021
View reviewed changes
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.

Looks perfect now, very flexible and consistent. But I would suggest bit different naming 🤗

mixin/README.md Outdated Show resolved Hide resolved
mixin/README.md Outdated Show resolved Hide resolved
mixin/README.md Outdated Show resolved Hide resolved
@kakkoyun
Address review issues
2c0af34 
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
bwplotka
bwplotka approved these changes Mar 19, 2021
View reviewed changes
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.

Epic, looks clean and amazing to me! 💪🏽 Let's go! 🚀

@bwplotka bwplotka merged commit f06e0b0 into thanos-io:main Mar 19, 2021
@kakkoyun kakkoyun deleted the mixin_add_alert_namespaces branch March 19, 2021 14:58
@dgrisonnet dgrisonnet mentioned this pull request Mar 26, 2021
Merged
2 tasks
@yeya24 yeya24 mentioned this pull request Nov 12, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka approved these changes

@metalmatze metalmatze Awaiting requested review from metalmatze

@squat squat Awaiting requested review from squat

@brancz brancz Awaiting requested review from brancz

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
@kakkoyun @brancz @bwplotka