🌱 Log full object name and creation error for topology template reconcile #7295
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
killianmuldoon
force-pushed
the
logs/log-full-object-name-for-templates
branch
from
da05e3a to
5ea7dbc
Compare
k8s-ci-robot
added
size/S
| func createErrorWithoutObjectName(err error, obj client.Object) error { | ||
| func createErrorWithoutObjectName(ctx context.Context, err error, obj client.Object) error { | ||
| log := ctrl.LoggerFrom(ctx) | ||
| log.Info("Failed to create object", "object", klog.KObj(obj), "error", err.Error()) |
There was a problem hiding this comment.
I think the simplest solution is just a generic log every time this function is called, but I'm open to opinions on when this might not be desired.
There was a problem hiding this comment.
I think thats fair, except it not being obvious to log the error due to the function name 🤔
But createAndLogErrorWithoutObjectName is also weird.
There was a problem hiding this comment.
Can we fix this by using log.WithCallDepth(1)?
If we can get this fixed I'm oke with the solution given that is scoped to a single file/private, but we should add one comment explaining that we log here in order to surface the name somewhere (without triggering reconcile)
There was a problem hiding this comment.
What about the following?
log.WithCallDepth(1).Error(err, "Failed to create object", obj.GetObjectKind().GroupVersionKind().Kind, klog.KObj(obj))- uses the Kind instead of just object (I think it should be always set, we set it explicitly in MD and all the unstructured stuff needs it). Would be good to double-check though.
- WithCallDepth(1) should work, but please verify
- I would use .Error instead of adding a error k/v pair
There was a problem hiding this comment.
The Call Depth is already set here as this case is using the topology logger (which sets its own call level). Right now it's logging the line number where the function call occurs which is the desired behaviour.
I've updated to use Kind (which I'm mixed on because object makes it clearer to me which object the creation error refers to in this line) and changed to .Error() in the latest version.
killianmuldoon
force-pushed
the
logs/log-full-object-name-for-templates
branch
from
5ea7dbc to
c91ae75
Compare
Signed-off-by: killianmuldoon <[email protected]>
killianmuldoon
force-pushed
the
logs/log-full-object-name-for-templates
branch
from
c91ae75 to
6997cf6
Compare
|
/lgtm /cherry-pick release-1.2 |
|
@fabriziopandini: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you. 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. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@fabriziopandini: new pull request created: #7352 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. |
Signed-off-by: killianmuldoon [email protected]
Add a log with the full object name in before stripping the unique identifier in createErrorWithoutObjectName.
Fixes #7238