Skip to content
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

Kubernetes 1.24: contextual logging blog post #304

Merged

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Apr 28, 2022

Contextual logging is an enhancement that landed as alpha in Kubernetes
1.24. Because it targets primarily developers of Kubernetes at this point,
publishing it under https://k8s.dev/blog on the same day as it also gets
published on the main blog (see
kubernetes/website#32656) makes sense.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 28, 2022
@pohly
Copy link
Contributor Author

pohly commented Apr 28, 2022

/hold

Because of #281

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 28, 2022
Copy link
Contributor

@jberkus jberkus left a comment

Choose a reason for hiding this comment

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

A few minor suggestion, no blockers.

Group](https://github.com/kubernetes/community/blob/master/wg-structured-logging/README.md)
has added new capabilities to the logging infrastructure in Kubernetes
1.24. This blog post explains how developers can take advantage of those to
make log output more useful.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: put a call for "and you can join us and help" here too, not just at the bottom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will change it.


Now `<` and `>` markers and indention are used to ensure that splitting at a
klog header at the start of a line is reliable and the output remains readable
for humans:
Copy link
Contributor

Choose a reason for hiding this comment

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

so ... what happens if the message text contains a ">" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought they were indented differently - let me double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the value uses \t and the end marker a single space.

This isn't visible when looking at the diff, but renders okay when looking at the full file.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I see, so the delimiters are really "<\n" and "^>".

Probably does not need to be added to the blog post, then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or to be very precise, =<\n and ^ > (one space) with ^\t for the lines in the middle 😅

Agreed, this doesn't need to be in the blog post.


## Next steps

The Structured Logging WG is always looking for new contributors. The migration
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jberkus, pohly

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 Apr 29, 2022
@pohly pohly force-pushed the kubernetes-1.24-contextual-logging branch from 6c3fd4a to 409eabb Compare May 2, 2022 07:19
@pohly
Copy link
Contributor Author

pohly commented May 2, 2022

Comments addressed, please take another look.

@pohly pohly force-pushed the kubernetes-1.24-contextual-logging branch from 409eabb to 5607a77 Compare May 2, 2022 08:35
@pohly
Copy link
Contributor Author

pohly commented May 2, 2022

I have force-pushed an update to address the review feedback from kubernetes/website#32656 (review)

@jberkus
Copy link
Contributor

jberkus commented May 2, 2022

/lgtm

from me

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2022
@pohly pohly force-pushed the kubernetes-1.24-contextual-logging branch from 5607a77 to 30e7b6c Compare May 4, 2022 14:55
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2022
@pohly
Copy link
Contributor Author

pohly commented May 4, 2022

The markup was wrong in at least one place and line breaks in link text looked odd, at least on GitHub. I've reformatted those.

When this lands in the blog, paragraphs will get reflown, right? I thought GitHub also did that, but apparently not.

@sftim
Copy link
Contributor

sftim commented May 11, 2022

Bear in mind that due to #281 this needs to be approved and LGTMed and then on the day of publication (not before), unheld.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Here's a few, small / not-merge-blocking, bits of feedback.

content/en/blog/2022/2022-05-25-contextual-logging.md Outdated Show resolved Hide resolved
content/en/blog/2022/2022-05-25-contextual-logging.md Outdated Show resolved Hide resolved
content/en/blog/2022/2022-05-25-contextual-logging.md Outdated Show resolved Hide resolved
[Kubernetes uses it now](https://github.com/kubernetes/kubernetes/commit/17e3c555c5115f8c9176bae10ba45baa04d23a7b),
or get invoked stand-alone.

We are in the process of moving the tool into a [new repository](https://github.com/kubernetes/klog/issues/312) because it isn't
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We are in the process of moving the tool into a [new repository](https://github.com/kubernetes/klog/issues/312) because it isn't
We are in the process of [moving the tool]((https://github.com/kubernetes/klog/issues/312)
into a new Git repository, because it isn't

Previous link text suggested it would be a link to the new repo, but actually wasn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepted with minor syntax fix.

content/en/blog/2022/2022-05-25-contextual-logging.md Outdated Show resolved Hide resolved
Comment on lines 145 to 160
- [`FromContext`](https://pkg.go.dev/k8s.io/klog/v2#FromContext): from a
`context` parameter, with fallback to the global logger
- [`Background`](https://pkg.go.dev/k8s.io/klog/v2#Background): the global
fallback, with no intention to support contextual logging
- [`TODO`](https://pkg.go.dev/k8s.io/klog/v2#TODO): the global fallback, but
only as a temporary solution until the function gets extended to accept
a logger through its parameters
- [`SetLoggerWithOptions`](https://pkg.go.dev/k8s.io/klog/v2#SetLoggerWithOptions):
changes the fallback logger; when called with
[`ContextualLogger(true)`](https://pkg.go.dev/k8s.io/klog/v2#ContextualLogger),
the logger is ready to be called directly, in which case logging will be done
without going through klog
Copy link
Contributor

Choose a reason for hiding this comment

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

These could use a definition list if you want to. See https://www.markdownguide.org/extended-syntax/#definition-lists for syntax that CommonMark is happy with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, I keep forgetting that definition lists sometimes work in Markdown. Changed.

The `example` prefix and `foo="bar"` were added by the caller of the function
which logs the `runtime` message and `duration="1m0s"` value.

An example for a unit test with per-test output is [included in klog](https://github.com/kubernetes/klog/blob/v2.60.1/ktesting/example/example_test.go).
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also write, eg:

Suggested change
An example for a unit test with per-test output is [included in klog](https://github.com/kubernetes/klog/blob/v2.60.1/ktesting/example/example_test.go).
The sample code for klog includes an an
[example](https://github.com/kubernetes/klog/blob/v2.60.1/ktesting/example/example_test.go)
for a unit test with per-test output.

which uses the active voice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepted with fix for "an an".

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yep. Thanks.

Contextual logging is an enhancement that landed as alpha in Kubernetes
1.24. Because it targets primarily developers of Kubernetes at this point,
publishing it under https://k8s.dev/blog on the same day as it also gets
published on the main blog (see
kubernetes/website#32656) makes sense.
@pohly pohly force-pushed the kubernetes-1.24-contextual-logging branch from 6d42cbf to 11c75c2 Compare May 12, 2022 09:01
@pohly
Copy link
Contributor Author

pohly commented May 19, 2022

@sftim, @jberkus : we are getting closer to the intended publishing data (25th of May). Is this good to go as it is?

Can we merge it now or unhold it on the day when it is supposed to go live (#281)?

@sftim
Copy link
Contributor

sftim commented May 19, 2022

We wouldn't merge it now. We should get the approvals and reviews in place so that an unhold on the day of publication is all ready to go.

@sftim
Copy link
Contributor

sftim commented May 19, 2022

We wouldn't merge it now. We should get the approvals and reviews in place so that an unhold on the day of publication is all ready to go.

Changes since #304 (comment) look minor.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2022
@sftim
Copy link
Contributor

sftim commented May 19, 2022

Is kubernetes/website#32656 level with the article text as reviewed here?

@pohly
Copy link
Contributor Author

pohly commented May 19, 2022

Is kubernetes/website#32656 level with the article text as reviewed here?

Not yet, but as this seems to be the final version now I'll copy it over.

@pohly
Copy link
Contributor Author

pohly commented May 25, 2022

/hold cancel

Let's publish...

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2022
@k8s-ci-robot k8s-ci-robot merged commit 9b1ee18 into kubernetes:master May 25, 2022
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants