I've been meaning to address this. You're right that the way we're using logrus doesn't really fit for CLI output. I think we can continue using centralized logging interfaces and satisfy both logging and CLI use cases.

For this particular logFatalf case, we could configure a logger instance distinct from the package-level logger, to output without formatting:

type cliFmt struct{}
func (f *cliFmt) Format(entry *log.Entry) ([]byte, error) {
	return []byte(entry.Message + "\n"), nil
}

func logFatalf(format string, v ...interface{}) {
	var cliLog = log.New()
	cliLog.Out = os.Stderr
	cliLog.Formatter = new(cliFmt)
	cliLog.Fatalf(format, v...)
}

Taking this a step further, we could instantiate a shared cliLog logger, and use it with the same pattern that we do with the package-level log lines.

I actually like the formatted output, but if we're going to change this then I'd prefer to have a global log object that is configured this way on root. Whatever solution we end up with, I think the important thing is that either we only use the wrapper functions or we only use the log object directly, which means that we'd need wrapped functions for all levels and formatted vs. ln versions.

Cool, yeah this all makes sense. I've removed the logFatalf method, and changed the initializer back to setting the log level on logrus's global logger. Related to my comment above, I've also defaulted the log level to "panic" (no logging), when the --verbose flag is not set. As part of this change, I've converted all of the log.Fatal calls in this package to instead print directly to stderr (since they otherwise wouldn't be printed at the default level). This guarantees that we won't print log messages unless the --verbose flag is set, and when we do print messages, they'll be fully formatted with timestamps.

TIOLI, I prefer the granular control that log levels provide, though I may be in the minority. @pcalcado and I discussed this on a previous PR: #154 (comment)

As another example, I needed exactly this functionality today while debugging with kubectl proxy --v=10:

$ kubectl options
...
  -v, --v=0: log level for V logs

Perhaps this may be symptom of trying to solve two distinct use cases: 1) verbose information for users vs. 2) debugging information for developers. I think the line between the two cases (and users) are blurry enough that multiple log levels can satisfy both.

Although I still prefer --verbose for the UX I'd be happier keeping the --log-level as it is right now. If anything, we could add a --verbose that pretty much is an alias for --log-level DEBUG later.

Ah, sorry, I missed the discussion about this in #154. I agree with @pcalcado's comments in that PR. Exposing log levels to a CLI feels unintuitive to me, since I usually expect long-running processes to log, and I expect CLI commands to print stuff and exit, but not log in the traditional sense. That said, for the sake of debugging, I can totally see the value in adjusting the verbosity of the command, which, again, doesn't really feel like logging to me.

Put another way, under normal circumstances, a CLI command should never log. It should only return exactly the output that it's intended to return, explicitly. Outputting messages as the byproduct of log statements seems messy. If the CLI does have the capacity to log, I'd expect the default log level to be "off". In that world, the -log-level flag would only ever be used to lower the log level for the sake of debugging. Hence my preference to switch this to a boolean --verbose flag that sets log-level=debug.

can we call this --proxy-log-level? with our usage of --log-level in other go processes, i'd prefer to be explicit that this something different from the log level for the process being executed.

also, could we provide some examples for valid inputs? for example, i used this this week: trace,conduit_proxy=trace,hyper=trace.

Yeah, so the way the RUST_LOG environment variable works is that it can take a comma separated list of { level | scope | scope=level }. A bare log level will enable that level for all modules, a bare scope will enable all log levels for that scope, and scope=level will set the log level for that scope to the given level.

Valid log levels (see https://docs.rs/log/0.4.1/log/#use) are:

• trace
• debug
• info
• warn
• error

The scopes we're probably the most likely to care about are:

• conduit_proxy: the proxy itself
• h2, tower-h2: the libraries we use for HTTP/2
• hyper: the library we used for HTTP/1
• tower-grpc: the gRPC library (probably will want the h2 libs as well for debugging gRPC issues)

You can set more granular scopes as well, but this is based on the layout of the Rust modules in the proxy.

For more in depth docs, see https://doc.rust-lang.org/log/env_logger/index.html#enabling-logging

Hmm, it occurs to me that this wall of text could probably go in docs somewhere now that I think of it.

It seems like we might be overthinking this a bit. The help message for this flag looks like this:

      --proxy-log-level string          log level for the proxy (default "warn,conduit_proxy=info")

Including "warn,conduit_proxy=info" as the default gives the user enough of an indication that you can set both a global log level and a package-specific log level. Anybody who needs more control than that is likely familiar enough with the proxy to know which levels and packages to use. So I'm inclined to rename this flag to --proxy-log-level and leave it as is, if that works for you all.

similar comment to above, can we use --controller-log-level?

I actually like the formatted output, but if we're going to change this then I'd prefer to have a global log object that is configured this way on root. Whatever solution we end up with, I think the important thing is that either we only use the wrapper functions or we only use the log object directly, which means that we'd need wrapped functions for all levels and formatted vs. ln versions.

Although I still prefer --verbose for the UX I'd be happier keeping the --log-level as it is right now. If anything, we could add a --verbose that pretty much is an alias for --log-level DEBUG later.

🚫 🌈 😢

Ah, you're right -- it turns out we don't actually need to disable colors. Will remove this.

curious: in this context, how do you think about the difference between:

return fmt.Errorf(...

and:

fmt.Fprintf(os.Stderr, ...
os.Exit(1)

This is a good question. If the run function returns an error, then cobra will print the error text and the usage message for the command that's being run. In most cases that's the right behavior (e.g. like on line 23 of this file, where the --proxy-port flag is invalid). But in this case, the usage checks have passed and something else has failed, in which case I think it's preferable to just print the failure and exit.

Makes sense, thanks for the explanation!