-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
contextual logging: beta in Kubernetes 1.30 #45288
Conversation
👷 Deploy Preview for kubernetes-io-vnext-staging processing.
|
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.
Please also update content/en/docs/reference/command-line-tools-reference/feature-gates/contextual-logging.md
@@ -133,8 +133,8 @@ If developers use additional functions like `WithValues` or `WithName` in | |||
their components, then log entries contain additional information that gets | |||
passed into functions by their caller. | |||
|
|||
Currently this is gated behind the `StructuredLogging` feature gate and | |||
disabled by default. The infrastructure for this was added in 1.24 without | |||
Currently this is gated behind the `ContextualLogging` feature gate and |
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.
Currently this is gated behind the `ContextualLogging` feature gate and | |
For Kubernetes {{< skew currentVersion >}}, this is gated behind the `ContextualLogging` | |
[feature gate](/docs/reference/command-line-tools-reference/feature-gates/) and is |
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.
Done.
I also updated the feature gate description.
Hello @pohly 👋 please take a look at Documenting for a release - PR Ready for Review to get your PR ready for review before Tuesday March 12th 2024 18:00 PST. Thank you! |
2d3a3a9
to
fcbeb85
Compare
contextual logging add extra detail to log output. | ||
When you disable this feature gate, Kubernetes components that support | ||
contextual logging might produce less detailed log output because | ||
some extra information might be provided only as part of the context. |
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.
Note that I switched from "if you enable" to "if you disable" here because that is what users would do now.
Also note that when converting existing log calls we try to avoid information loss should the feature get disabled. But in new code that I recently wrote for DRA, I am assuming that the feature remains enabled and avoid logging information both via context and as parameters.
I expect this to happen more often going forward because the duplication is annoying.
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.
We always write descriptions as what happens when the feature is enabled.
Try
Enables additional detail in log output from Kubernetes components that
support writing structure logs.
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.
Think about the graduation to stable; Kubernetes still has the feature, but now you cannot disable it. We want a feature description that makes sense for beta and for stable.
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.
Makes sense. I used the text that I had in the main file because this works regardless of the log output format.
Enables extra details in log output of Kubernetes components that support
contextual logging.
/milestone 1.30 |
Hello @pohly 👋, it looks like you've already engaged feedback in review of this PR. Great job! Keep this up. 🚀 @alculquicondor @liggitt @serathius @ehashman @thockin It also looks like there is a hold here and a PR that has been merged. Can this hold be removed? Thanks! |
There's not much technical content that needs to be reviewed here. I think it's fine if the docs team does a LGTM + approve for the wording and formatting. /hold cancel Implementation was merged, PR can be merged, too. |
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.
One thing to fix, otherwise LGTM
contextual logging add extra detail to log output. | ||
When you disable this feature gate, Kubernetes components that support | ||
contextual logging might produce less detailed log output because | ||
some extra information might be provided only as part of the context. |
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.
We always write descriptions as what happens when the feature is enabled.
Try
Enables additional detail in log output from Kubernetes components that
support writing structure logs.
fcbeb85
to
4f0dc7a
Compare
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.
/lgtm
for docs
/approve
I don't think this needs a technical review before it merges.
@@ -122,7 +122,7 @@ second line.} | |||
|
|||
### Contextual Logging |
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.
(nit / good to fix before GA / yes, I know the rest of the file is also not following the style guide here)
### Contextual Logging | |
### Contextual logging |
LGTM label has been added. Git tree hash: 98023378d82e6b99b4d3f4589d7ec6037bd36753
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sftim 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 |
/milestone 1.30 |
Related-to: kubernetes/enhancements#3077
/hold
kubernetes/kubernetes#122589 needs to be merged first.