-
Notifications
You must be signed in to change notification settings - Fork 545
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 more logging around catalog source sync #3414
base: master
Are you sure you want to change the base?
Add more logging around catalog source sync #3414
Conversation
a5ec49a
to
e13881e
Compare
@@ -897,6 +897,11 @@ func (o *Operator) handleCatSrcDeletion(obj interface{}) { | |||
} | |||
|
|||
func validateSourceType(logger *logrus.Entry, in *v1alpha1.CatalogSource) (out *v1alpha1.CatalogSource, continueSync bool, _ error) { | |||
logger = logger.WithField("step", "validateSourceType") | |||
logger.Info("validate source type start") |
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 in practice you'll find method book-ending is extremely chatty - on the order of MiB/s logging rate during high churn, especially when the system is just verifying correctness of current state and not changing. Not sure if you really require it. The logs for error cases and outcomes are very useful and will not emit if "nothing" is going on.
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.
yeah - I tend to agree. The verbosity is high for low utility. I'll cull most of the non-error logs. At least in these paths, anyway. Wondering if just leaving pre-return log would still be a good idea to get signal that it went into the method - even if only for the methods higher up the stack....?
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.
okie - see if that sparks joy, or if you reckon it's still too verbose
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.
Try it out and see! It might still be too chatty to be useful, but you can't know without running an end-to-end test.
d6c2abb
to
b48b163
Compare
5d8932d
to
dad1e2a
Compare
Signed-off-by: Per Goncalves da Silva <[email protected]>
dad1e2a
to
576ea51
Compare
Description of the change:
A recent live debugging session revealed that the catalog source reconciliation loop is under logged. This PR adds more logging. Generally, I've tried to:
Motivation for the change:
Architectural changes:
Testing remarks:
Reviewer Checklist
/doc
[FLAKE]
are truly flaky and have an issue