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

An extra line break was output #370

Closed
aimuz opened this issue Feb 17, 2023 · 7 comments · Fixed by #378
Closed

An extra line break was output #370

aimuz opened this issue Feb 17, 2023 · 7 comments · Fixed by #378
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@aimuz
Copy link

aimuz commented Feb 17, 2023

/kind bug

What steps did you take and what happened:
[A clear and concise description of what the bug is.]

This is a log without SetLogger

I0217 18:17:05.411664   62273 shared_informer.go:280] Caches are synced for project-controller
I0217 18:17:05.411821   62273 shared_informer.go:280] Caches are synced for application-controller

This is set by SetLogger

2023-02-17T18:18:12.680+0800    INFO    client-go       Waiting for caches to sync for application-controller

2023-02-17T18:18:12.680+0800    INFO    client-go       Waiting for caches to sync for project-controller

2023-02-17T18:18:12.781+0800    INFO    client-go       Caches are synced for project-controller

2023-02-17T18:18:12.781+0800    INFO    client-go       Caches are synced for application-controller


They output a new line between them. After my investigation, I found that klog adds a new line at the end of the msg, but for the log output by the logr, the new line will be automatically added.

He can work normally under stdout, because stdout needs this newline

https://github.com/kubernetes/klog/blob/main/klog.go#L729

What did you expect to happen:

Do not add new lines when using logr

2023-02-17T18:18:12.680+0800    INFO    client-go       Waiting for caches to sync for application-controller
2023-02-17T18:18:12.680+0800    INFO    client-go       Waiting for caches to sync for project-controller
2023-02-17T18:18:12.781+0800    INFO    client-go       Caches are synced for project-controller
2023-02-17T18:18:12.781+0800    INFO    client-go       Caches are synced for application-controller

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 17, 2023
@aimuz
Copy link
Author

aimuz commented Feb 17, 2023

If you accept this problem, can you assign it to me? I am interested in solving this problem

@pohly
Copy link

pohly commented Feb 17, 2023

Which logr.Logger implementation is this? If a trailing newline in the message string leads to undesired output, can't that implementation strip it out?

There's nothing in the go-logr API that says that the message string shouldn't end in a newline. As it's unspecified, I would expect the LogSink to deal well with both cases (newline and no newline).

@aimuz
Copy link
Author

aimuz commented Feb 19, 2023

Which logr.Logger implementation is this? If a trailing newline in the message string leads to undesired output, can't that implementation strip it out?

I don't think it's a question of whether to delete it or not, it's not normal behavior for a log library to always delete the last newline (which itself needs to output newlines?)

The problem with klog is that it adds newlines to all logs. For example, if you want to output "hello world", klog will process it as "hello world\n" and then call the logr implementation. It is normal to use the logr implementation because stdout or stderr require newlines, but that is only their

Now the logic

step1: klog.Info("hello world")
step2: msg = msg + "\n"
step3: stdout.W(msg) or logr.Info(msg)

He thought their ideal logic would be something like this

step1: klog.Info("hello world")
step3: stdout.W(msg+"\n") or logr.Info(msg)

@pohly
Copy link

pohly commented Feb 20, 2023

If it's not making the code flow more complex in klog, then I am open to merging a PR which solves this in klog. But if it's complex, then the question stands whether a solution elsewhere might be possible.

@logicalhan
Copy link
Member

/triage accepted
/assign @pohly

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 23, 2023
@pohly
Copy link

pohly commented May 9, 2023

/unassign
/help-wanted

@pohly
Copy link

pohly commented May 9, 2023

/assign @aimuz

aimuz added a commit to aimuz/klog that referenced this issue May 11, 2023
klog always adds a newline to the msg. klog will work fine without klog.

Set logr with klog.SetLogger, and klog will also pass the msg with the newline added to logr, which will result in an accidental newline being added.

step1: klog.Info("hello world")
step2: msg = msg + "\n"
step3: stdout.W(msg) or logr.Info(msg[:len(msg)-1])

fix kubernetes#370

Signed-off-by: aimuz <mr.imuz@gmail.com>
aimuz added a commit to aimuz/klog that referenced this issue May 11, 2023
klog always adds a newline to the msg. klog will work fine without klog.

Set logr with klog.SetLogger, and klog will also pass the msg with the newline added to logr, which will result in an accidental newline being added.

step1: klog.Info("hello world")
step2: msg = msg + "\n"
step3: stdout.W(msg) or logr.Info(msg[:len(msg)-1])

fix kubernetes#370

Signed-off-by: aimuz <mr.imuz@gmail.com>
aimuz added a commit to aimuz/klog that referenced this issue May 11, 2023
klog always adds a newline to the msg. klog will work fine without klog.

Set logr with klog.SetLogger, and klog will also pass the msg with the newline added to logr, which will result in an accidental newline being added.

step1: klog.Info("hello world")
step2: msg = msg + "\n"
step3: stdout.W(msg) or logr.Info(msg[:len(msg)-1])

fix kubernetes#370

Signed-off-by: aimuz <mr.imuz@gmail.com>
aimuz added a commit to aimuz/klog that referenced this issue May 11, 2023
klog always adds a newline to the msg. klog will work fine without klog.

Set logr with klog.SetLogger, and klog will also pass the msg with the newline added to logr, which will result in an accidental newline being added.

step1: klog.Info("hello world")
step2: msg = msg + "\n"
step3: stdout.W(msg) or logr.Info(msg[:len(msg)-1])

fix kubernetes#370

Signed-off-by: aimuz <mr.imuz@gmail.com>
aimuz added a commit to aimuz/klog that referenced this issue May 17, 2023
klog always adds a newline to the msg. klog will work fine without klog.

Set logr with klog.SetLogger, and klog will also pass the msg with the newline added to logr, which will result in an accidental newline being added.

step1: klog.Info("hello world")
step2: msg = msg + "\n"
step3: stdout.W(msg) or logr.Info(msg[:len(msg)-1])

fix kubernetes#370

Signed-off-by: aimuz <mr.imuz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants