-
Notifications
You must be signed in to change notification settings - Fork 380
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
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
@pavolloffay All comments already addressed! |
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
@pavolloffay this is ready for another review. Thank you! |
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.
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.
AFAIK, the warnings will be printed but the collector will be created anyway. Those are warnings but no errors.
Update: after taking a second read, I would say @rubenvp8510 is right and the collector will not be 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
2c35147
to
cec000f
Compare
modeMetricName = prefix + "info" | ||
) | ||
|
||
// TODO: Refactor this logic, centralize it. |
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.
Can you create a task for this or link the current task where this work will be done?
@pavolloffay @jaronoff97 Any other observation? |
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
65aa48c
to
b60e4c8
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>
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>
Co-authored-by: Israel Blancas <iblancasa@gmail.com>
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>
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: