config: Flag to disable access logger

[LINKIWI]

The access logger can be quite noisy when bazel-remote is serving thousands of HTTP requests. This PR proposes the addition of a configuration switch to disable the access logger.

The changes:

• Attach the access and error *log.Logger instances to the Config struct
• Move all logging instantiation logic to a new file config/logger.go [+]
• Add a new configuration param disable_access_log available both as a CLI flag and in the YAML configuration to disable the access logger

[+] This approach, I think, makes it easier to make further customizations to the logging behavior in the future. For example, this PR doesn't touch the behavior of the error logger, but I suppose there could be a use case for suppressing those messages as well.

[+] This is also required because the current implementation (on master) creates the loggers before the config is read. This change proposes creating the loggers after the config is read because we need to have a parsed config to decide how to create the loggers.

Test:

Help text:

$ bazelisk run //:bazel-remote -- --help
...
   --disable_access_log  Whether to disable the standard output access logger (default: false, ie enable access logging) [$BAZEL_REMOTE_DISABLE_ACCESS_LOG]

Default behavior (no change compared to current master):

$ bazelisk run //:bazel-remote -- --dir /tmp/bazel --max_size 1 --port 8080
...
2021/05/12 00:18:52  GET 200             ::1 /cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

$ curl -i localhost:8080/cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
HTTP/1.1 200 OK
...

Disabled access logger via CLI flag:

$ bazelisk run //:bazel-remote -- --dir /tmp/bazel --max_size 1 --port 8080 --disable_access_log

$ curl -i localhost:8080/cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
HTTP/1.1 200 OK
...

Disabled access logger via config file:

$ cat config.yaml
dir: /tmp/bazel
max_size: 1
port: 8080
disable_access_log: true
$ bazelisk run //:bazel-remote -- --config_file $(pwd)/config.yaml

$ curl -i localhost:8080/cas/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
HTTP/1.1 200 OK
...

[ulrfa]

I typically redirect the access log on stdout to /dev/null, and log stderr to file.

For me the possibility to enable/disable access log from configuration file, could be useful if combined with support for reloading configuration file via SIGHUP, as discussed in #352

[mostynb]

Thanks for the PR @LINKIWI.

This is related to #408 (FYI @glukasiknuro).

I like these changes, but I would like to take some time to think through the command line flag/config file setting before merging. In particular I wonder if it would be better to use a more flexible setting than a boolean? Let's discuss some options before implementing them.

One possibility I have been thinking about recently:
--access-log-level <NONE | MOST | ALL> - where MOST could mean something like "truncate noisy FindMissingBlobs logs but include everything else"

@ulrfa: I have been doing some experimental refactoring that would make #352 a little easier to achieve. But it's still early days.

[LINKIWI]

I also like the idea of a leveled access logger. Happy to send a patch to this PR if that's the preferred direction.

In terms of implementation, there's a few ways I think is reasonable:

• Including a numerical level in Config and placing if configured level > None/Most/All/etc { accesslogger.Printf() } at callsites
• Creating multiple *log.Logger instances (one per level), overriding Write logic to respect the configured logging level, and modifying callsites to use the appropriately leveled access logger for each call
• Migrating to a third party logging library like zap et al. that handles most of these with built-in abstractions. This approach feels a bit heavyweight and probably outside of the scope of changes I'm comfortable making.

[mostynb]

If we were to add --access-log-level <"none" | "all" (default)> now, then we would have the option to add a "most" value later. Apart from changing the flag and the config file setting this PR could mostly be kept as it is.

Or we could use integers, but then we should define three levels now: --access-log-level <0 | 1 | 2 (default)> (and make 1 and 2 both mean "all" for now).

I think I prefer the string values better (though we could store the value as an integer internally). What does everyone else think?

[LINKIWI]

I'm +1 to --access-log-level <"none" | "all" (default)>. It leaves room for future extensibility without requiring major changes to the proposed business logic.

[mostynb]

OK- let's go ahead with this.

[LINKIWI]

Changes:

• Change flag from disable_access_logger to access_log_level
• Add a simple check in validateConfig that a supported log level is supplied
• Change the logging instantiation business logic to instead discard the output when the level is "none"

I didn't create constants for "none" and "all" since there doesn't seem to be a convention for that (see e.g. "zstd"/"uncompressed") but I can do that if needed.

Additional manual verification:

$ bazelisk run //:bazel-remote -- --dir /tmp/bazel --max_size 1 --port 8080 --access_log_level invalid
...
2021/05/14 18:37:07 bazel-remote built with go1.16.2 from git commit ec16031f6133df98481eb48df8137d1204ba2b63.
'access_log_level' must be set to either "none" or "all"
...

[mostynb]

Thanks- squashed and added to the master branch.