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

Monitoring of custom resources #133

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

sebastiangaiser
Copy link

This proposal is related to strimzi/strimzi-kafka-operator#10276.
As this is my first proposal, so please let me know if any changes (also to the format) are needed.

@im-konge @scholzj can you please help me filling the TODO parts.

Thank you for your help and your work on this project.

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal. I tried to answer the TODOs and left some more minor comments. But I think it looks good overall. Thanks.

086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
086-monitoring-of-custom-resources.md Show resolved Hide resolved
086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
@ppatierno
Copy link
Member

@sebastiangaiser hi! are you still working on this proposal and planning to take care of Jakub's comments?

@sebastiangaiser
Copy link
Author

@ppatierno yes I'm still interested, sorry for the delay. I hope to find the time this week.

sebastiangaiser and others added 2 commits October 17, 2024 20:45
Co-authored-by: Jakub Scholz <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
@sebastiangaiser sebastiangaiser force-pushed the modocs/nitoring-of-custom-resources branch from 9791fe7 to 6c13bf3 Compare October 17, 2024 18:55
@sebastiangaiser
Copy link
Author

@scholzj thank you for your suggestions and comments. I've tried to address them. Please let me know if something is missing or needs more clarification.

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I think this looks good now and we should open it for a general review. Unless you plan any more changes, yan you make it Ready for Review and I can request the reviews. Thanks.

@sebastiangaiser sebastiangaiser marked this pull request as ready for review October 21, 2024 07:19
@scholzj scholzj changed the title docs(proposal): monitoring of custom resources Monitoring of custom resources Oct 21, 2024
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

I left a couple of comments.

Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

Thanks @sebastiangaiser, I left a few questions.

086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
086-monitoring-of-custom-resources.md Outdated Show resolved Hide resolved
This also applies for PrometheusRules which should be replaced.
The proposed way would be implementing the KSM and deprecating the current metrics in Strimzi CO in version 0.45 and removing them in version 0.49 to give users enough time adjusting their monitoring if needed.
So there is no immediate impact for users and enough time for migration.

Copy link
Member

Choose a reason for hiding this comment

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

I think you should say explicitly that this proposal is changing what's responsible for providing the metrics, but that the metrics provided by KSM will be compatible with the metrics that were provided by the CO. I.e. same metrics names and tags, measuring the same things.

I guess one question I have is for people who are using Strimzi and the metrics it currently provides, but who might not themselves have the ability to enable the addon required by KSM. Does anyone know how commonly the addon is enabled in pratice?

Copy link
Author

Choose a reason for hiding this comment

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

I guess one question I have is for people who are using Strimzi and the metrics it currently provides, but who might not themselves have the ability to enable the addon required by KSM. Does anyone know how commonly the addon is enabled in pratice?

I'm not sure if I got you here but KSM is neither an addon to Kubernetes nor to Strimzi. It's an application exposing the status (and other parts) of Kubernetes resources. So you could deploy KSM as a standalone application with access to the Kubernetes API server 😉

Copy link
Author

Choose a reason for hiding this comment

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

Ah they specify it as Add-on agent to generate and expose cluster-level metrics., sorry for that. But in the end you could e.g. have that for multiple different tools using custom resources. E.g. one KSM deployment for Strimzi, one for Flux, ...

Co-authored-by: Tom Bentley <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks. This LGTM.

sebastiangaiser and others added 2 commits October 30, 2024 09:14
Co-authored-by: Paolo Patierno <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Copy link
Member

@im-konge im-konge left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

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.

5 participants