-
Notifications
You must be signed in to change notification settings - Fork 57
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
base: main
Are you sure you want to change the base?
Monitoring of custom resources #133
Conversation
Signed-off-by: Sebastian Gaiser <[email protected]>
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.
Thanks for the proposal. I tried to answer the TODOs and left some more minor comments. But I think it looks good overall. Thanks.
@sebastiangaiser hi! are you still working on this proposal and planning to take care of Jakub's comments? |
@ppatierno yes I'm still interested, sorry for the delay. I hope to find the time this week. |
Co-authored-by: Jakub Scholz <[email protected]> Signed-off-by: Sebastian Gaiser <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
9791fe7
to
6c13bf3
Compare
Signed-off-by: Sebastian Gaiser <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
@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. |
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.
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.
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.
I left a couple of comments.
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.
Thanks @sebastiangaiser, I left a few questions.
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. | ||
|
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.
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?
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.
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 😉
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.
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]>
Signed-off-by: Sebastian Gaiser <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
Signed-off-by: Sebastian Gaiser <[email protected]>
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.
Thanks. This LGTM.
Signed-off-by: Sebastian Gaiser <[email protected]>
Co-authored-by: Paolo Patierno <[email protected]> Signed-off-by: Sebastian Gaiser <[email protected]>
…example Signed-off-by: Sebastian Gaiser <[email protected]>
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, 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.
LGTM. 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.
LGTM, thanks a lot!
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.