-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 logging to Enqueue_owner event handler #2427
✨ add logging to Enqueue_owner event handler #2427
Conversation
This PR adds logging to Enqueue owner event handler, that is helpful in logging the GVK of the owner that caused reconcilation. Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: varshaprasad96 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -467,7 +491,7 @@ var _ = Describe("Eventhandler", func() { | |||
{ | |||
Name: "foo-parent", | |||
Kind: "HorizontalPodAutoscaler", | |||
APIVersion: "autoscaling/v2beta1", | |||
APIVersion: "autoscaling/v2", |
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.
Bumping this since v2beta1 was deprecated since k8s 1.22
"Owner Name", ownerReference.Name, | ||
) | ||
|
||
log.Info("OwnerReference handler event", kvs...) |
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.
What "level" does this get logged at? Is there a way for me to specify that I only want this logged at a certain verbosity level?
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.
log.Info("OwnerReference handler event", kvs...) | |
log.V(1).Info("OwnerReference handler event", kvs...) |
I don't think this should be logged at the normal Info level, as it will create too chatty logs. See https://github.com/kubernetes-sigs/controller-runtime/blob/main/TMP-LOGGING.md
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, this is defininitely debug level, it will cause a huge amount of logs for a controller that is even slightly busy
"Owner Name", ownerReference.Name, | ||
) | ||
|
||
log.Info("OwnerReference handler event", kvs...) |
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.
log.Info("OwnerReference handler event", kvs...) | |
log.V(1).Info("OwnerReference handler event", kvs...) |
I don't think this should be logged at the normal Info level, as it will create too chatty logs. See https://github.com/kubernetes-sigs/controller-runtime/blob/main/TMP-LOGGING.md
@@ -197,3 +201,49 @@ func (e *enqueueRequestForOwner) getOwnersReferences(object metav1.Object) []met | |||
// No Controller OwnerReference found | |||
return nil | |||
} | |||
|
|||
// logEvent logs an event with the version, kind and name of the object and its owner. | |||
func (e *enqueueRequestForOwner) logEvent(eventType string, object, newObject client.Object) { |
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 am personally not a big fan of func signatures like this one - with positional parameters. Could using the builder pattern be an option here?
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.
Agreed. But a builder pattern just for logEvent
could be an overkill just for one helper. I'll rework on this bit, to fetch the owner references from both the objects and then pass it to logevent so that its a bit more cleaner.
@@ -34,9 +35,10 @@ func TestEventhandler(t *testing.T) { | |||
|
|||
var testenv *envtest.Environment | |||
var cfg *rest.Config | |||
var logBuffer bytes.Buffer |
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.
Could this global variable cause issues since it's mutated (Reset
) in the test specs? How about unit testing the logging in enqueueRequestForOwner
instead of in the e2e-tests?
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.
How about unit testing the logging in enqueueRequestForOwner instead of in the e2e-tests?
The logger is currently getting tested while unit testing for specific events in enqueueRequestForOwner
. Could you point to the alternate option?
Could this global variable cause issues since it's mutated (Reset) in the test specs?
Would prefer to keep this at the beginning of test suite, because it needs to be set as the default logger before starting the tests. BeforeSuite
anyways gets executed only once, though we are resetting the content quite a few times in the test spec for specific tests.
@@ -112,6 +115,7 @@ func (e *enqueueRequestForOwner) Delete(ctx context.Context, evt event.DeleteEve | |||
|
|||
// Generic implements EventHandler. | |||
func (e *enqueueRequestForOwner) Generic(ctx context.Context, evt event.GenericEvent, q workqueue.RateLimitingInterface) { | |||
e.logEvent("Generic", evt.Object, nil) |
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.
It adds some amount of processing to a code path, which is frequently called. I am wondering whether it would not be possible to leverage what is already returned by getOwnerReconcileRequest
a couple of lines below to get information on the owner instead of what is made in logEvent
and extractTypedOwnerReference
?
Also wouldn't we want to provide information on the object itself?
@varshaprasad96 sorry, I don't understand the problem you are solving. How does the implementation being internal prevent you from adding logging? Your wrapper can and should wrap the I am not a huge fan of this because it opens a can of worms:
I'd prefer it if we made it easy to wrap a handler, possibly even provide a generic logging implementation but not adjust all the handlers. |
I agree with @alvaroaleman, and to get a bit more familiar with the c-r codebase, I drafted something that might allow users to wrap event handlers. Feel free to use it in a new PR if you like the idea: 2f01213 |
@alvaroaleman Agreed that we need to implement the handler interface. However,
I would like to explore the options before closing the PR here:
I'm not super familiar with the client-go side of code, but the question which I have with (1) and (3) is while wrapping or enabling multiple handlers, there is no guarantee on the order in which listeners will be distributed. Which is something we would want to maintain at least while logging for a particular event? |
Yeah, you can do that as a wraper.
The approach of injecting things is deprecated. You can get a logger from the
I don't understand this part. The source registers the eventhandler in the cache and then passed the event onto the predicate.predicate and finally handler.Handler (the package here). Why do you want multiple handlers? |
I'm inclined to close this PR given no recent activity and the alternative suggested by @alvaroaleman. @varshaprasad96 WDYT? |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/close per comment above |
@vincepri: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This PR adds logging to Enqueue owner event handler, that is helpful in logging the GVK of the owner that
caused reconciliation.
Fixes: #1186
In Operator SDK we have a
LoggingEnqueueRequestForOwnerHandler
(ref) that used to add logging to the existingEnqueueRequestForOwnerHandler
. In C-R 0.15.0 this was made internal, making it difficult to create a wrapper around it. However, adding logging to the existing handler available in c-r would be useful to other consumers also in identifying the cause of reconcile.