logging: include information about contextual logging #6560
Conversation
|
/wg structured-logging |
| logging](https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/3077-contextual-logging#transition). It | ||
| also lists several | ||
| [pitfalls](https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/3077-contextual-logging#pitfalls-during-usage) | ||
| that developers and reviewers need to be aware of. |
There was a problem hiding this comment.
I decided against copying the text because then we would have to maintain it in two places. Can we assume that readers of this document here will follow the links?
contributors/devel/sig-instrumentation/migration-to-structured-logging.md
Show resolved
Hide resolved
|
|
||
| Calling `ErrorS` with `nil` as error is semi-acceptable if there is error condition that deserves a stack trace at this |
There was a problem hiding this comment.
Printing a stack trace is not the main difference between InfoS and ErrorS, and neither text nor JSON output include a stack trace for ErrorS.
I'm wondering whether that should be documented, because if a stack trace is desired, then the suggested replacement is loosing that information. In kubernetes/klog#316 I am proposing some new helper functions that could be used to address this gap. Should we try to get that into Kubernetes 1.24 together with FlushAndExit?
There was a problem hiding this comment.
When writing this document we wanted a clear way for users to decide whether they should use Info or Error. An error that doesn't happen during routine operation and deserves a stacktrace was a distinction proposed by @thockin. In the end didn't implement writing stacktrace.
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
area/developer-guide
k8s-ci-robot
added
sig/instrumentation
|
/hold Code PRs needs to be merged first. |
k8s-ci-robot
added
the
do-not-merge/hold
|
/assign |
|
/hold cancel Code PRs are all merged, contextual logging is alpha in 1.24. Let's finish the documentation... 😁 |
k8s-ci-robot
removed
the
do-not-merge/hold
| ## Migration | ||
|
|
||
| 1. Change log functions to structured equivalent | ||
| 1. Find code locations that need to be changed: |
There was a problem hiding this comment.
I think that this tool would be better to verify the results than find code locations.
There was a problem hiding this comment.
Could make it part of Verify log output?
There was a problem hiding this comment.
I think that this tool would be better to verify the results than find code locations.
Then how do you suggest that developers should find code locations instead? Using "git grep" easily leads to false positives. The logcheck tool is more precise.
Could make it part of Verify log output?
That section is about something else (the output that gets generated, not how it gets generated). I can add a "Preventing regressions" section and there describe how we use hack/logcheck.conf to ensure that undesirable calls do not get added back into already converted packages.
|
I think there will always be times when I have an "error situation"
(something bad happened, but I am not reporting it further up-stack - maybe
I will retry or something) but I do not have a literal `error` type
variable. `log.ErrorS()` is still appropriate, IMO
…On Mon, Apr 4, 2022 at 8:48 AM Marek Siarkowicz ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
contributors/devel/sig-instrumentation/migration-to-structured-logging.md
<#6560 (comment)>:
>
-Calling `ErrorS` with `nil` as error is semi-acceptable if there is error condition that deserves a stack trace at this
When writing this document we wanted a clear way for users to decide
whether they should use Info or Error. An error that doesn't happen during
routine operation and deserves a stacktrace was a distinction proposed by
@thockin <https://github.com/thockin>. In the end didn't implement
writing stacktrace.
—
Reply to this email directly, view it on GitHub
<#6560 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVAXUFNGFKFC22GS6TLVDMFNVANCNFSM5ROOZNDA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
And that's why both |
pohly
force-pushed
the
contextual-logging
branch
from
c5eba40
to
6e7ae08
Compare
| updated. If there are any `context.TODO` calls in the modified functions, | ||
| replace with the new `ctx` parameter. | ||
|
|
||
| In performance critical code it may be faster to add a `logger klog.Logger` |
There was a problem hiding this comment.
Do you have any hard numbers on this?
There was a problem hiding this comment.
I did some microbenchmarking for the KEP: kubernetes/enhancements#3078 (comment)
Those tests are pending for Kubernetes: https://github.com/kubernetes/kubernetes/pull/109194/commits
contributors/devel/sig-instrumentation/migration-to-structured-logging.md
Outdated
Show resolved
Hide resolved
In Kubernetes 1.24, the necessary infrastructure got added. Now developers can take advantage of it.
pohly
force-pushed
the
contextual-logging
branch
from
19067cd
to
94e8428
Compare
k8s-ci-robot
added
the
lgtm
|
/assign @nate-double-u |
|
Not sure if I'm able to approve in this repo, but this looks good to me. |
|
/assign @serathius For final review and approval. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nate-double-u, pohly, serathius 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
In Kubernetes 1.24, the necessary infrastructure got added. Now developers can
take advantage of it.