Skip to content

Conversation

@serathius
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented May 26, 2020

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit f9833ee

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5f0ce2b316c4c9000893182f

@k8s-ci-robot k8s-ci-robot requested review from piosz and x13n May 26, 2020 12:34
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels May 26, 2020
@sftim
Copy link
Contributor

sftim commented May 26, 2020

/milestone 1.19

@k8s-ci-robot k8s-ci-robot added this to the 1.19 milestone May 26, 2020
@fejta-bot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/check-cla

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 26, 2020
@sftim
Copy link
Contributor

sftim commented Jun 21, 2020

@serathius because of the switch to Docsy I recommend that you rebase this against dev-1.19 once you've got something ready to review.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 3, 2020
@serathius serathius changed the title [WIP] Placeholder for Structured Logging Enhancement Placeholder for Structured Logging Enhancement Jul 3, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 3, 2020
@serathius
Copy link
Contributor Author

/cc @dims @44past4 @DirectXMan12

@serathius serathius changed the title Placeholder for Structured Logging Enhancement Document Structured Logging Enhancement Jul 3, 2020
@celestehorgan
Copy link

/assign

Copy link

@celestehorgan celestehorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good first pass! Lots of comments, mostly around grammar. Thank you for writing this!


By controlling verbosity (using `-v` command line flag) we can filter logs based on how severe they are, controlling resulting log volume.
The higher verbosity of log the less severe they are, with verbosity 0 being used for logs that should ALWAYS be visible.
The practical default level is 2.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What is the full range of numbers you can use with -v? (it starts at 0, 2 is "practical" for daily use... but how high can I go?)
  2. How do I use -v – i.e. -v=0?
  3. Is there any way we can describe what gets added to the logs with each number increment? i.e.
  • 0 = essential messages, like system errors
  • 1 =
  • 2 = lists successful events (or something)
  • 3 =
  • 4 =

etc.?

if this is impractical feel free to ignore it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ad 1, don't think there is a limit, guidelines give usecase up to 5, highest that make sense was 10 (there is 12, but looks like mistake).
Ad 2 Yes,
Ad 3 We have guide for developers (linked in what's next). But I would not suggest to detail levels in documentation as I don't think they strictly complied to, so we shouldn't promise it to users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help a lot to specify that logging verbosity is an integer, and to provide a range.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serathius: I would take the list from the doc you just linked and include it in this documentation :). This is valuable information!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc is old developers guide, some parts are outdated. As this is guidelines, I don't expect them to be very accurate. I would be worried about adding this to documentation.

Verbosity level values are a evolving and can we cannot give any guarantees about where what type of log is where (in current state). Creating a guide was meant to give contributors some intuition about levels, but not hard rules.

For example, this guide lists HTTP logs on V(2), but I'm pretty sure that they are on V(3) currently. Copy&Pasting this guide will result in bad experience of users, when they will try to base their configuration on incorrect information in guide.

Overall I think we should convey that users should experiment with levels and select one based on costs they are willing to pay for log volume generated.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay then. For now let's go with that approach :)

Comment on lines 88 to 90
By controlling verbosity (using `-v` command line flag) we can filter logs based on how severe they are, controlling resulting log volume.
The higher verbosity of log the less severe they are, with verbosity 0 being used for logs that should ALWAYS be visible.
The practical default level is 2.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
By controlling verbosity (using `-v` command line flag) we can filter logs based on how severe they are, controlling resulting log volume.
The higher verbosity of log the less severe they are, with verbosity 0 being used for logs that should ALWAYS be visible.
The practical default level is 2.
The `-v` flag controls log verbosity. Setting the value higher increases the events logged, and decreasing it lowers the events logged.
The higher verbosity of log the less severe they are, with 0 for events which should always be logged.
In most cases, it's safe to use the default value, 2.

Question: is 2 the default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serathius
Copy link
Contributor Author

Done

@serathius
Copy link
Contributor Author

ping @kbhawkey

@kbhawkey
Copy link
Contributor

kbhawkey commented Jul 9, 2020

@serathius , I added a few more comments about tidying up the text.
Hello @dims @44past4 @DirectXMan12. This pull request is getting close to being ready.
Would you have time to review? Thanks!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2020
@44past4
Copy link
Contributor

44past4 commented Jul 13, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2020
@savitharaghunathan
Copy link
Member

Hi @serathius 👋 , Friendly reminder that docs should be reviewed and ready to be merged by July 16th. Thanks!

* Separate system logs from Logging Architecture as it mainly covers application logging
* Rename "Metics For The Kubernetes Control Plane" to "Metrics For The Kubernetes System Components" as it covers non-control plane components
* Rename files to `system-logs.md` and `system-metrics.md`
* Remove trailing whitespaces
* Add sections about Klog and Structured Logging

Co-authored-by: Celeste Horgan <[email protected]>
Co-authored-by: Zach Corleissen <[email protected]>
Co-authored-by: Tim Bannister <[email protected]>
Co-authored-by: Paweł Kępka <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2020
@kbhawkey
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2020
@dims
Copy link
Member

dims commented Jul 14, 2020

/lgtm

Copy link
Member

@savitharaghunathan savitharaghunathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: savitharaghunathan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2020
@k8s-ci-robot k8s-ci-robot merged commit 61ec4a4 into kubernetes:dev-1.19 Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.