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
Add crd metrics usage information #2825
base: main
Are you sure you want to change the base?
Conversation
@rubenvp8510 thanks for opening this up. Could you open an issue first so we could discuss how we could improve the observability about the operator? |
Also changelog is missing |
@jaronoff97 Created the issue and linked here. @pavolloffay Added changelog. |
8956946
to
37e2e95
Compare
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Hello @pavolloffay @jaronoff97 sorry for the delay on this PR This is how the metrics looks like with the latest changes
Let me know if this is satisfactory and if you have more observations on this PR. Thank you very much for all the reviews! At the end I use a gauge and the webhooks to detect creation/deletion/update, as Pavol suggested. It is a clean solution IMHO. |
3225b3a
to
9e4448b
Compare
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@pavolloffay All comments already addressed! |
Signed-off-by: Ruben Vargas <ruben.vp8510@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. I would like to see some tests for the Metrics
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@pavolloffay this is ready for another review. Thank you! |
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Co-authored-by: Israel Blancas <iblancasa@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
warnings, err := c.validate(ctx, otelcol) | ||
if err != nil { | ||
return warnings, err | ||
} | ||
|
||
if c.metrics != nil { | ||
c.metrics.update(ctx, otelcolOld, otelcol) | ||
} |
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.
We probably should do the metrics prior to returning, otherwise if anyone has any warnings in their CR we won't record the metrics
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.
But the return above (line 195) only happens if validation errors are returned, right? I'm assuming if errors were found, the collector is not created.
warnings, err := c.validate(ctx, otelcol) | ||
if err != nil { | ||
return warnings, err | ||
} | ||
|
||
if c.metrics != nil { | ||
c.metrics.delete(ctx, otelcol) | ||
} |
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.
same as above
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
2c35147
to
cec000f
Compare
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Description:
Add a new package that collect different metrics about the collectors on the cluster.
Link to tracking Issue(s):
Testing:
Documentation: