logging: add way to configure logging level via cilium-agent option #8607

ianvernon · 2019-07-17T21:53:04Z

The new option can be provided as follows to cilium-agent:

--log-opt level=error

If no option is set, or the provided option does not match a level for logrus,
use the default logging level (info level). If debug mode is not enabled,
endpoint loggers will use the logging level provided via the command-line
option, if there was one provided. If not, the endpoitn logger will default
to info level as well.

I added this because I thought I would be able to inject the logging level for
unit testing via a -X parameter when running go test, but given how we
set up our loggers (in their own package), that didn't work very well. But,
I didn't want my work to go to waste, so I posted this PR.

Signed-off by: Ian Vernon ian@cilium.io

This change is

ianvernon · 2019-07-17T22:00:22Z

test-me-please

pkg/endpoint/log.go

ianvernon · 2019-07-17T22:10:01Z

test-me-please

ianvernon · 2019-07-17T22:52:33Z

test-me-please

ianvernon · 2019-07-18T16:54:00Z

test-me-please

ianvernon · 2019-07-18T23:31:45Z

test-me-please

ianvernon · 2019-07-19T20:11:41Z

test-me-please

coveralls · 2019-07-19T20:35:56Z

Coverage increased (+0.06%) to 44.177% when pulling 111c71f on pr/ianvernon/log-level-argument into b6a1790 on master.

aanm

Where is the option flag being set in the cilium-agent?

pkg/logging/logging.go

ianvernon · 2019-07-23T00:13:35Z

Where is the option flag being set in the cilium-agent?

@aanm it is set as a --log-opt, which is a mapping of key-value pairings for configuration related to logging.

pkg/logging/logging.go

ianvernon · 2019-07-23T18:14:01Z

test-me-please

ianvernon · 2019-07-23T18:15:30Z

test-me-please

joestringer

Looks fine to me. The logic seems like something easy to unit-test (debug overrides other options, invalid options specification is overridden with default, etc).

Also not sure why the options need to be cached in a var, since it seems like they're just validated and configured into the logging library. But this should be inconsequential.

ianvernon · 2019-07-24T17:59:45Z

@joestringer will add some unit tests.

Re: why options are cached in a var - outside packages consume the configuration (specifically the endpoint log). The value has to be inspected as well when we are changing from user-specified log level --> debug --> user-specified log level as well.

ianvernon · 2019-07-24T18:34:40Z

test-me-please

ianvernon · 2019-07-24T18:35:00Z

@joestringer added the unit tests - created a type wrapping around the logging options to make unit testing easier.

Addressed comments.

pkg/logging/logging.go

raybejjani · 2019-07-31T09:30:28Z

pkg/logging/logging.go

+// configureLogLevelFromOptions sets the log level of the DefaultLogger based
+// off of the value of LevelOpt in logOpts. If LevelOpt is not set in logOpts,
+// it defaults to DefaultLogLevelStr.
+func setLogLevelFromOptions(logOpts LogOptions) {


It feels a little odd to have this private function but a global GetLogLevel that is more generic. Creating a SetLogLevel(logrus.Level) feels more re-usable. It is just shuffling things around, though, since we only use this in one callpath.

raybejjani · 2019-07-31T09:41:05Z

pkg/logging/logging.go

 // It is thread-safe.
-func ToggleDebugLogs(debug bool) {
+func ConfigureLogLevel(debug bool) {


When I saw the name change I was expecting this function to take in both debug and options. It feels weird to mix using the global and the debug parameter. Is there any chance we can pass in the configuration here every time? That will probably mean exposing options in some way but you might have done all this to avoid that?

I can't import pkg/option into here because it will result in a dependency loop. Perhaps I can do it as an interface.

Oh, I meant that we were referring to the global variable options in this package while also taking the debug parameter. Both are configuration to be parsed/used. To be clear, I'm fine with the global variable. I would just prefer to pass in both debug and options here.
I don't fully see why pkg/options would come up in this case; all calls to ConfigureLogLevel could pass in the LogOptions type (SetupLogging already does this).

The weird thing about debug is that it's extracted from other configuration outside of the logging package. What I'd love for us to have (which would be a significant change) is to be able to register objects to be updated when configuration that could be changed after Cilium is started (e.g., cilium config x=y), and that way we'd not have to pass debug around as a variable in function calls (not sure how much that makes sense).

Logging level is an option configured at agent bootstrap vs. debug being a configurable option that can change during a single run of a given agent, which is why debug is the only "variable" here.

raybejjani

Looking at this again, nothing is blocking. I left some clarifications on my comments but we can change things later if you agree, or not if you don't.

pkg/logging/logging.go

raybejjani · 2019-08-05T13:52:12Z

pkg/logging/logging.go

 // It is thread-safe.
-func ToggleDebugLogs(debug bool) {
+func ConfigureLogLevel(debug bool) {


Oh, I meant that we were referring to the global variable options in this package while also taking the debug parameter. Both are configuration to be parsed/used. To be clear, I'm fine with the global variable. I would just prefer to pass in both debug and options here.
I don't fully see why pkg/options would come up in this case; all calls to ConfigureLogLevel could pass in the LogOptions type (SetupLogging already does this).

ianvernon · 2019-08-05T16:41:02Z

test-me-please

The new option can be provided as follows to `cilium-agent`: ``` --log-opt level=error ``` If no option is set, or the provided option does not match a level for logrus, use the default logging level (info level). If debug mode is not enabled, endpoint loggers will use the logging level provided via the command-line option, if there was one provided. If not, the endpoitn logger will default to info level as well. If debug mode is enabled, it will override the value passed for the log level. Signed-off by: Ian Vernon <ian@cilium.io>

ianvernon · 2019-08-12T15:55:17Z

test-me-please

ianvernon added kind/enhancement This would improve or streamline existing functionality. wip labels Jul 17, 2019

ianvernon requested review from a team as code owners July 17, 2019 21:53

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from 8ab4bda to 965fef1 Compare July 17, 2019 22:08

ianvernon commented Jul 17, 2019

View reviewed changes

pkg/endpoint/log.go Outdated Show resolved Hide resolved

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from 965fef1 to 097bd63 Compare July 17, 2019 22:52

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from 097bd63 to 5c170c8 Compare July 18, 2019 23:31

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from 5c170c8 to 430750c Compare July 19, 2019 15:21

ianvernon added pending-review and removed wip labels Jul 19, 2019

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from 430750c to 7b11f62 Compare July 19, 2019 20:11

aanm previously requested changes Jul 22, 2019

View reviewed changes

nebril previously requested changes Jul 22, 2019

View reviewed changes

pkg/logging/logging.go Outdated Show resolved Hide resolved

pkg/logging/logging.go Outdated Show resolved Hide resolved

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from 7b11f62 to fe421fd Compare July 23, 2019 18:13

ianvernon requested a review from a team July 23, 2019 18:13

houndci-bot reviewed Jul 23, 2019

View reviewed changes

pkg/logging/logging.go Show resolved Hide resolved

ianvernon requested review from aanm and nebril July 23, 2019 18:13

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from fe421fd to c0e5ca0 Compare July 23, 2019 18:15

joestringer approved these changes Jul 24, 2019

View reviewed changes

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from c0e5ca0 to 76ef141 Compare July 24, 2019 18:34

joestringer approved these changes Jul 25, 2019

View reviewed changes

raybejjani suggested changes Jul 31, 2019

View reviewed changes

raybejjani approved these changes Aug 5, 2019

View reviewed changes

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from 76ef141 to 111c71f Compare August 5, 2019 16:40

ianvernon force-pushed the pr/ianvernon/log-level-argument branch from 111c71f to ed4fa78 Compare August 12, 2019 15:55

ianvernon merged commit 77583f4 into master Aug 12, 2019

ianvernon deleted the pr/ianvernon/log-level-argument branch August 12, 2019 18:10

ianvernon added this to the 1.7 milestone Aug 12, 2019

aanm added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Nov 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logging: add way to configure logging level via cilium-agent option #8607

logging: add way to configure logging level via cilium-agent option #8607

ianvernon commented Jul 17, 2019 •

edited

ianvernon commented Jul 17, 2019

ianvernon commented Jul 17, 2019

ianvernon commented Jul 17, 2019

ianvernon commented Jul 18, 2019

ianvernon commented Jul 18, 2019

ianvernon commented Jul 19, 2019

coveralls commented Jul 19, 2019 •

edited

aanm left a comment

ianvernon commented Jul 23, 2019

ianvernon commented Jul 23, 2019

ianvernon commented Jul 23, 2019

joestringer left a comment

ianvernon commented Jul 24, 2019

ianvernon commented Jul 24, 2019

ianvernon commented Jul 24, 2019 •

edited

raybejjani Jul 31, 2019

raybejjani Jul 31, 2019

ianvernon Jul 31, 2019

raybejjani Aug 5, 2019

ianvernon Aug 5, 2019

raybejjani left a comment

raybejjani Aug 5, 2019

ianvernon commented Aug 5, 2019

ianvernon commented Aug 12, 2019

logging: add way to configure logging level via cilium-agent option #8607

logging: add way to configure logging level via cilium-agent option #8607

Conversation

ianvernon commented Jul 17, 2019 • edited

ianvernon commented Jul 17, 2019

ianvernon commented Jul 17, 2019

ianvernon commented Jul 17, 2019

ianvernon commented Jul 18, 2019

ianvernon commented Jul 18, 2019

ianvernon commented Jul 19, 2019

coveralls commented Jul 19, 2019 • edited

aanm left a comment

Choose a reason for hiding this comment

ianvernon commented Jul 23, 2019

ianvernon commented Jul 23, 2019

ianvernon commented Jul 23, 2019

joestringer left a comment

Choose a reason for hiding this comment

ianvernon commented Jul 24, 2019

ianvernon commented Jul 24, 2019

ianvernon commented Jul 24, 2019 • edited

raybejjani Jul 31, 2019

Choose a reason for hiding this comment

raybejjani Jul 31, 2019

Choose a reason for hiding this comment

ianvernon Jul 31, 2019

Choose a reason for hiding this comment

raybejjani Aug 5, 2019

Choose a reason for hiding this comment

ianvernon Aug 5, 2019

Choose a reason for hiding this comment

raybejjani left a comment

Choose a reason for hiding this comment

raybejjani Aug 5, 2019

Choose a reason for hiding this comment

ianvernon commented Aug 5, 2019

ianvernon commented Aug 12, 2019

ianvernon commented Jul 17, 2019 •

edited

coveralls commented Jul 19, 2019 •

edited

ianvernon commented Jul 24, 2019 •

edited