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

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

Merged
merged 18 commits into from
Mar 19, 2021

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Mar 1, 2021

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
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
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 kakkoyun changed the title mixin: Add namespace to the alerts mixin: Introduce flexible multi-cluster/namespace mode for alerts and dashboards Mar 5, 2021
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
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?

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.

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
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.

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>
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.

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
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.

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
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.

3 participants