-
Notifications
You must be signed in to change notification settings - Fork 133
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
webhook: switch to contextual logger #1295
Conversation
/cc @lburgazzoli @zdtsw |
I can add "webhook" to the name, may be it will make filtering a bit easy( .WithName("webhook") should make "logger": "admission.webhook"). What do you think?
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## incubation #1295 +/- ##
=============================================
Coverage ? 19.24%
=============================================
Files ? 30
Lines ? 2676
Branches ? 0
=============================================
Hits ? 515
Misses ? 2099
Partials ? 62 ☔ View full report in Codecov by Sentry. |
+1 to add |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zdtsw 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 |
/onhold |
did not see the comments that you wanna add the name |
cce5d3f
to
0211239
Compare
New changes are detected. LGTM label has been removed. |
0211239
to
5c7d60b
Compare
Updated. Wondering, would it be better to move newLogConstructor to logger package. |
bf3ab29
to
7c99412
Compare
/cc @bartoszmajsak |
LGTM, I let @zdtsw for final review |
i am fine with the change, only one part i am not sure is |
Good point. Will "DefaultingWebhook" work? |
7c99412
to
7943dc0
Compare
Use k8s contextual logger instead of own. Comparing to other parts of the operator webhook is a bit special since it serves own endpoints and has context independent from controllers. Include more info (operation), just to get more details. Do not add kind since it's clear from "resource" field. Since controller-framework adds "admission" to the name, use own LogConstructor with own base logger for the framework's DefaultLogConstructor. Add name locally to distinguish from framework's messages. Add Name field to the structures to avoid copying string literal for both WithName() and WithValues(). The output changes and it looks like ``` {"level":"info","ts":"2024-10-11T05:17:20Z","logger":"ValidatingWebhook","msg":"Validation request","object":{"name":"default-dsci"},"namespace":"","name":"default-dsci","resource":{"group":"dscinitialization.opendatahub.io","version":"v1","resource":"dscinitializations"},"user":"kube:admin","requestID":"e5bf3768-6faa-4e14-9004-e54ee84ad8b7","webhook":"ValidatingWebhook","operation":"CREATE"} ``` or for the defaulter: ``` {"level":"info","ts":"2024-10-11T04:50:48Z","logger":"DefaultingWebhook","msg":"Defaulting DSC","object":{"name":"default-dsc"},"namespace":"","name":"default-dsc","resource":{"group":"datasciencecluster.opendatahub.io","version":"v1","resource":"datascienceclusters"},"user":"kube:admin","requestID":"c9213ff3-80ee-40c0-9f15-12188dece68e","webhook":"DefaultingWebhook"} ``` (the messages are not from the current codebase, was added for demo only) Signed-off-by: Yauheni Kaliuta <[email protected]>
7943dc0
to
f989eec
Compare
/unhold |
Jira: https://issues.redhat.com/browse/RHOAIENG-14419
Use k8s contextual logger instead of own. Comparing to other parts of the operator webhook is a bit special since it serves own endpoints and has context independent from controllers.
Include more info (operation), just to get more details. Do not add kind since it's clear from "resource" field.
Since controller-framework adds "admission" to the name, use own LogConstructor with own base logger for the framework's DefaultLogConstructor.
Add name locally to distinguish from framework's messages.
Add Name field to the structures to avoid copying string literal for both WithName() and WithValues().
The output changes and it looks like
or for the defaulter:
(the messages are not from the current codebase, was added for demo only)
Description
How Has This Been Tested?
Screenshot or short clip
Merge criteria