search

sifive / wit

Workspace Integration Tool
Apache License 2.0
23 stars 13 forks source link

Add --quiet option, disables logging #214

Closed mmjconolly closed 4 years ago

jackkoenig commented 4 years ago

Two questions:

  1. Should --quiet also suppress log.error? It currently does in this PR.
  2. Should we have a way of setting the log level to warn?
richardxia commented 4 years ago

Two questions:

  1. Should --quiet also suppress log.error? It currently does in this PR.
  2. Should we have a way of setting the log level to warn?

In general, the logging levels are a bit unintuitive to me since there are so many of them (Python's built-in 5 plus 3 more) for a relatively small program. Do we have any guidelines as to when to use the following levels?

Regardless of my question above, my suggestion is that --quiet should suppress "WARNING" and lower, and that I'm fine with not having a way of setting the logging level to WARNING. I probably would have suggested that -v meant INFO level and higher, rather than VERBOSE level and higher.

jackkoenig commented 4 years ago

Sam and I may have gone a bit crazy with the number of levels, but the basic idea of the additional 2 levels (trace and spam) is that they're for developer-level printing. Whereas the normal 5 are for users to understand what's going on and thus should be logging of events or information that should be exposed to the user, trace and spam are for developers to add information for really picking through what happened.

We made info the default level because it's like the output of a command. What log level should the output of wit status be otherwise? Warn?

I am fine with your suggestions though. I guess warn and info are kind of similar and go together, it's just nice to have the addition [warn] to bring the users attention to stuff while the info level doesn't have any marker/prefix.

richardxia commented 4 years ago

Caveat: The following reflects my personal opinions, which have been highly influenced by a relatively small number of codebases, generally coming from a Python-based and Web-based world, where logging information is often collected into a centralized logging server. I am not suggesting that we change anything at the moment, but I felt that it would be useful to for me to explain my biases and preconceptions. I have found it useful myself to be exposed to the Java conventions for logging levels, since I have not had much experience with them.

Sam and I may have gone a bit crazy with the number of levels, but the basic idea of the additional 2 levels (trace and spam) is that they're for developer-level printing. Whereas the normal 5 are for users to understand what's going on and thus should be logging of events or information that should be exposed to the user, trace and spam are for developers to add information for really picking through what happened.

Speaking from a Python perspective, and primarily from a web development background, I normally view DEBUG as being closer to what you are describing as "developer-level" info. For what it's worth, the Java library log4j describes INFO, DEBUG, and TRACE as the following:

The INFO level designates informational messages that highlight the progress of the application at coarse-grained level.

The DEBUG Level designates fine-grained informational events that are most useful to debug an application.

The TRACE Level designates finer-grained informational events than the DEBUG

I am somewhat equating "developer-level" with "useful to debug an application", but I think it's generally the case that the two go hand in hand.

We made info the default level because it's like the output of a command. What log level should the output of wit status be otherwise? Warn?

I wouldn't normally consider INFO to be like the "output of a command". I am actually used to leaving off INFO-level logging by default, and forcing someone to opt in to it, hence my suggestion above to define -v as INFO-level or higher.

I would actually claim that anything that is actually the output of a command should go directly onto standard output, since I normally feel that "logging" is auxiliary information. For a command like wit status, where the main desired outcome is not a mutation of (filesystem) state but rather the printing of information, I think it's acceptable for us to not print it through a logging API. Another way of thinking about it is that even if you wanted to silence all logging, it never makes sense to silence the primary output of wit status.

I am fine with your suggestions though. I guess warn and info are kind of similar and go together, it's just nice to have the addition [warn] to bring the users attention to stuff while the info level doesn't have any marker/prefix.

I think in terms of logging levels, WARN/WARNING and INFO have different semantics, where WARNINGs are generally things that indicate a potential problem or an unexpected condition, but that are not necessarily expected to cause an error, while INFOs are merely informational statements that do not imply anything about correctness.

jackkoenig commented 4 years ago

A bit late on my response but I'm fine with adopting what you've proposed @richardxia.

The one caveat is that a good reason to have standard output go through the logger is it provides a more functional API that you could use for unit testing (you capture output into your logging object). That being said, we don't really follow the IO monad pattern since we're globally materializing the logger rather than passing it as an argument, and we don't follow that pattern for anything else, so I'm not sure that desired and as of yet unused feature is worth deviating from standard Python practice. Furthermore, even if we continue to go through the logger, we could have a method defined stdout rather than having it on the same spectrum as logging.