Implement leveled logging by utkuozdemir · Pull Request #895 · metallb/metallb

utkuozdemir · 2021-06-18T16:42:02Z

Signed-off-by: Utku Ozdemir uoz@protonmail.com

This PR implements leveled logging. I set the levels as I've seen fit by looking at their messages - any feedbacks are appreciated.
Fixes #254
Fixes #655

gclawes · 2021-06-18T20:52:18Z

Should we update the helm charts and manifests to add the --log-level flag?

utkuozdemir · 2021-06-18T21:53:54Z

Should we update the helm charts and manifests to add the --log-level flag?

Agreed and added.

What do you think about using info as the default value? On one hand it's a reasonable default, on the other hand if you want to keep the logging behavior unchanged, all or debug can do that.

champtar · 2021-06-19T00:58:06Z

On my phone, but I think I would bump many debug to info, all the ones that show a change in the system. The most spam comes from memberlist, then service sync I think.

utkuozdemir · 2021-06-19T10:39:01Z

@champtar
Thanks. Please see my latest commit which bumps some of the logs from debug to info.
With this change, debug level is used for logs that tell about:

the starts and ends of operations
no-ops ("nothing to do"s)

The operations that inform about a change/update etc. are now using the info level.

champtar

Some nitpicks
After those changes LGTM

utkuozdemir · 2021-07-08T12:27:02Z

@champtar applied the changes you requested in the last 2 commits. Please check.

champtar · 2021-07-08T12:47:19Z

And if you can squash your commits a bit that would be perfect :)

champtar

not tested but LGTM !

utkuozdemir

thanks

champtar · 2021-07-12T16:32:55Z

@russellb did you delete some of your reviews ? I got email notification that I can't find in here

russellb · 2021-07-12T17:13:49Z

@russellb did you delete some of your reviews ? I got email notification that I can't find in here

yeah, sorry. Commented on some, changed my mind, deleted, and now going back through and will post them all together as a single "review". I probably should have just replied to myself on here.

russellb

This looks good to me. I spotted one additional message I would have chosen debug, but the rest looked good. I started taking notes on messages I'd like to change in follow-ups but stopped since none of them are really related to these changes.

russellb · 2021-07-12T20:16:47Z

 				c.syncFuncs = append(c.syncFuncs, c.epInformer.HasSynced)
 			} else {
-				c.logger.Log("op", "New", "msg", "using endpoint slices")
+				level.Info(c.logger).Log("op", "New", "msg", "using endpoint slices")


I'd go with debug here

Endpoint slices are still new in MetalLB so it's a good information to have in bug report for now IMO.

(and it's just 1 line)

I guess so, but it's an implementation detail that shouldn't matter unless something is broken (like many other debug logs).

Like you said, it's just 1 line so I don't feel that strongly ...

Should I do this change? Or you can do it directly.

russellb · 2021-07-12T20:40:27Z

... CI doesn't seem to have run on this PR? I have no idea why.

Signed-off-by: Utku Ozdemir <uoz@protonmail.com>

russellb · 2021-07-12T20:47:52Z

I pushed a rebase of this PR to the branch to try to trigger CI and it seems to have worked

Signed-off-by: Utku Ozdemir <uoz@protonmail.com>

utkuozdemir · 2021-07-13T18:14:04Z

Some of my changes seems to have disappeared - I think it was caused by the rebase somehow, but not sure. I have (re)pushed those changes in the latest commit.

russellb · 2021-07-13T18:44:41Z

Some of my changes seems to have disappeared - I think it was caused by the rebase somehow, but not sure. I have (re)pushed those changes in the latest commit.

I apologize if I overwrote something. I thought I was up to date from your branch before I did the rebase. In any case, it seems CI is running (and passing) now. FWIW, I saw those same failures locally when I pulled down this PR to test it, so maybe those build fixes hadn't been pushed?

utkuozdemir · 2021-07-13T18:56:09Z

Some of my changes seems to have disappeared - I think it was caused by the rebase somehow, but not sure. I have (re)pushed those changes in the latest commit.

I apologize if I overwrote something. I thought I was up to date from your branch before I did the rebase. In any case, it seems CI is running (and passing) now. FWIW, I saw those same failures locally when I pulled down this PR to test it, so maybe those build fixes hadn't been pushed?

No problem at all.

I just think internal/speakerlist/log.go went missing, because I added it as @champtar 's request, and I think he saw it: #895 (comment)

Or it might be my squash that made it disappear 🤔

But anyway, doesn't matter - all back in their place now :)

russellb · 2021-07-13T19:15:16Z

I'm trying to build this locally to look at logs and it's failing. Any idea why?

$ inv dev-env -p layer2
kind version
kind v0.10.0 go1.15.7 linux/amd64
go build -v -o build/amd64/controller/controller -ldflags '-X go.universe.tf/metallb/internal/version.gitCommit=25e1592a -X go.universe.tf/metallb/internal/version.gitBranch=pr-895' go.universe.tf/metallb/controller
go.universe.tf/metallb/internal/logging
# go.universe.tf/metallb/internal/logging
internal/logging/logging.go:56:14: level.NewFilter undefined (type string has no field or method NewFilter)
internal/logging/logging.go:147:15: level.AllowAll undefined (type string has no field or method AllowAll)
internal/logging/logging.go:149:15: level.AllowDebug undefined (type string has no field or method AllowDebug)
internal/logging/logging.go:151:15: level.AllowInfo undefined (type string has no field or method AllowInfo)
internal/logging/logging.go:153:15: level.AllowWarn undefined (type string has no field or method AllowWarn)
internal/logging/logging.go:155:15: level.AllowError undefined (type string has no field or method AllowError)
internal/logging/logging.go:157:15: level.AllowNone undefined (type string has no field or method AllowNone)

$ go version
go version go1.16.4 linux/amd64

utkuozdemir · 2021-07-13T19:20:13Z

It was fixed by my last commit: 93208e2

Specifically, by the rename level -> lvl.

Can you delete your local branch and checkout again the remote branch? Might help.

russellb · 2021-07-13T19:24:03Z

It was fixed by my last commit: 93208e2

Specifically, by the rename level -> lvl.

Can you delete your local branch and checkout again the remote branch? Might help.

oops, you're right. :-D thanks

russellb · 2021-07-13T19:29:21Z

OK, last request, I think ... would you mind adding an entry to the "New Features" section of the release notes about this? I would note that leveled logging is now supported, what the default behavior using our helm chart or manifests, and how someone would go about changing it as needed.

See website/content/release-notes/_index.md

Signed-off-by: Utku Ozdemir <uoz@protonmail.com>

utkuozdemir · 2021-07-14T16:26:33Z

OK, last request, I think ... would you mind adding an entry to the "New Features" section of the release notes about this? I would note that leveled logging is now supported, what the default behavior using our helm chart or manifests, and how someone would go about changing it as needed.

See website/content/release-notes/_index.md

Done, but feel free to change the wording as you see fit.

russellb · 2021-07-14T17:55:22Z

OK, last request, I think ... would you mind adding an entry to the "New Features" section of the release notes about this? I would note that leveled logging is now supported, what the default behavior using our helm chart or manifests, and how someone would go about changing it as needed.
See website/content/release-notes/_index.md

Done, but feel free to change the wording as you see fit.

looks good to me. Thank you for all of your work!

utkuozdemir · 2021-07-14T17:57:06Z

Thanks! Happy to contribute :)

zigmund · 2021-07-16T07:43:51Z

@utkuozdemir, thanks for the fix.

@russellb, are you going to release this in near future?

russellb · 2021-07-16T12:35:44Z

I'll probably wait a bit. We just did a feature release (0.10) pretty recently, so I figure we'd wait a bit longer to get some other stuff in first.

cjreyn · 2021-09-15T08:48:00Z

Thank you for all the good work on Metallb. We're using it heavily to support science in the UK at Diamond Light Source!
Do you have any idea which release this PR is planned for?

curantes · 2021-09-15T14:20:05Z

Thank you for all the good work on Metallb. We're using it heavily to support science in the UK at Diamond Light Source!
Do you have any idea which release this PR is planned for?

v0.11.0 according to the milestone and release date have not been announced what I know of.

disclaimer: I am not part of metallb project

zigmund · 2021-09-15T16:59:28Z

It's really long-waited fix for big amounts of users, please release.

jonaz · 2021-10-19T06:28:54Z

Still get about 10 million logs per day from metallb so this would really offload our logging infrastructure! <3
Looking forward to having this feature released!

utkuozdemir requested review from daxmc99, johananl, rata and russellb as code owners June 18, 2021 16:42

champtar reviewed Jul 7, 2021

View reviewed changes

Comment thread internal/speakerlist/speakerlist.go Outdated

champtar suggested changes Jul 8, 2021

View reviewed changes

Comment thread controller/service.go Outdated

Comment thread controller/main.go

utkuozdemir force-pushed the leveled-logging branch from a70abc5 to a0f2162 Compare July 8, 2021 15:21

champtar approved these changes Jul 8, 2021

View reviewed changes

utkuozdemir commented Jul 10, 2021

View reviewed changes

russellb added this to the v0.11.0 milestone Jul 12, 2021

russellb reviewed Jul 12, 2021

View reviewed changes

Comment thread charts/metallb/values.yaml

champtar mentioned this pull request Jul 12, 2021

SpeakerList improvements #595

Closed

1 task

russellb reviewed Jul 12, 2021

View reviewed changes

Utku Ozdemir added 2 commits July 12, 2021 16:46

Implement leveled logging

8e63f44

Signed-off-by: Utku Ozdemir <uoz@protonmail.com>

Set log level to info on k8s manifests

77907ab

Signed-off-by: Utku Ozdemir <uoz@protonmail.com>

russellb force-pushed the leveled-logging branch from 25e1592 to 77907ab Compare July 12, 2021 20:47

Add missing changes to fix the build

93208e2

Signed-off-by: Utku Ozdemir <uoz@protonmail.com>

Update release notes for log level support

c77bece

Signed-off-by: Utku Ozdemir <uoz@protonmail.com>

russellb merged commit 95f6562 into metallb:main Jul 14, 2021

utkuozdemir deleted the leveled-logging branch July 14, 2021 17:58

github-actions Bot mentioned this pull request Mar 2, 2026

🌱 CNCF mission generation 2026-03-02 kubestellar/console-kb#251

Merged

Uh oh!

Conversation

utkuozdemir commented Jun 18, 2021

Uh oh!

gclawes commented Jun 18, 2021

Uh oh!

utkuozdemir commented Jun 18, 2021

Uh oh!

champtar commented Jun 19, 2021

Uh oh!

utkuozdemir commented Jun 19, 2021

Uh oh!

Uh oh!

champtar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

utkuozdemir commented Jul 8, 2021

Uh oh!

Uh oh!

Uh oh!

champtar commented Jul 8, 2021

Uh oh!

champtar left a comment

Choose a reason for hiding this comment

Uh oh!

utkuozdemir left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

champtar commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russellb commented Jul 12, 2021

Uh oh!

russellb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

russellb Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

champtar Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

champtar Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

russellb Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

utkuozdemir Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

russellb commented Jul 12, 2021

Uh oh!

russellb commented Jul 12, 2021

Uh oh!

utkuozdemir commented Jul 13, 2021

Uh oh!

russellb commented Jul 13, 2021

Uh oh!

utkuozdemir commented Jul 13, 2021

Uh oh!

russellb commented Jul 13, 2021

Uh oh!

utkuozdemir commented Jul 13, 2021

Uh oh!

russellb commented Jul 13, 2021

utkuozdemir left a comment •

edited

Loading

champtar commented Jul 12, 2021 •

edited

Loading