Skip to content

Commit

Permalink
feat: refactor logging
Browse files Browse the repository at this point in the history
  • Loading branch information
phillebaba committed Jun 12, 2024
1 parent 83da6d2 commit 3a9a87c
Showing 1 changed file with 69 additions and 0 deletions.
69 changes: 69 additions & 0 deletions adr/0025-logging.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# 25. Logging

Date: 2024-06-06

## Status

Proposed

## Context

Zarf is currently using an in-house built logging solution which in turn depends on pterm. This solution is used to output information to the end user who is using the Zarf CLI. This output is for the most part formatted with the purpose of look good for users of the CLI. Logging function calls are done both in the CLI specific code as well as in the packages. The logging is implemented in such a way that different levels exist with different impacts and log destinations. A common pattern that is used is to call `message.Fatal` whenever an error occurs. This will output the message to STDERR while writing the actual error message to a debug log and exit the program. Exiting the program in this manner makes unit testing difficult while also skipping proper context handling skipping any clean-up that was intended to run before exiting the program. Some logging components like the progress bar and spinner are accessed through a shared global state that is not thread safe. The coupling to logging becomes complicated as disabling the progress bar is a challenge while multi threading these functions which access the global state results in complicated access patterns.

## Decision

I am proposing to completely refactor the logging functionality to follow a more standardized format using the newish slog interfaces. On top of that we would refactor the current internationalization by converting them to standardized errors.

* Replace the message package with a slog interface.
* Replace the lang package with static errors.
* Remove any use of `message.Fatal` and instead return errors.
* Refactor use of `message.WarnErr` to either return error or to log the error.
* Refactor existing functions that print formatted outputs to defer the printing to the CLI.
* Define a new interface for spinner and progress bar that is passed in as a function parameter.

The message package currently exports the following functions which should be replaced by its logr counterpart.

| Function | Replacement | Comment |
| --- | --- | --- |
| ZarfCommand | Info | Just formats a slice as a command. |
| Command | Info | Outputs a command with a prefix and style. |
| Debug | Degub | |
| Debugf | Debug | |
| ErrorWebf | N/A | Not used anywhere. |
| Warn | Warn | |
| Warnf | Warn | |
| WarnErr | Warn | |
| WarnErrf | Warn | |
| Fatal | N/A | Should not be used. |
| Fatalf | N/A | Should not be used. |
| Info | Info | |
| Infof | Info | |
| Success | Info | Seems to be a info log with a checkmark prefix. |
| Successf | Info | Seems to be a info log with a checkmark prefix. |
| Question | ? | Open question how to resolve this. |
| Questionf | ? | Open question how to resolve this. |
| Note | Info | Should just be an info or maybe a debug log. |
| Notef | Info | Should just be an info or maybe a debug log. |
| Title | ? | Not sure how this should be replaced as it formats with separator. |
| HeaderInfof | ? | |
| HorizontalRule | ? | |
| JSONValue | N/A | Should be replaced with a marshal. |
| Paragraph | ? | |
| Paragraphn | ? | |
| PrintDiff | ? | |
| Table | ? | Need to come up with a good syntax for functions to return output that can print as a table. |
| ColorWrap | ? | Should this be used? |
| PrintConnectStringTable | N/A | This logic should not exist in the message package. |
| PrintCredentialTable | N/A | This logic should not exist in the message package. |
| PrintComponentCredential | N/A | This logic should not exist in the message package. |
| PrintCredentialUpdates | N/A | This logic should not exist in the message package. |
| Spinner | Interface | New Spinner interface. |
| ProgressBar | Interface | New progress bar interface. |

The majority of simpel logging changes should be possible with little signature changes. Replacing the existing output with a slog interface would allow other implementations to be used. A challenge initially may be the changes to table output formatting to make it work properly. This change will require some refactoring of existing code. A requirement for the changes is that they have to improve the UX for users looking at log files. As I understand the present day the spinners will cause a new line everytime they update, resulting in a lot of bloat. A goal should be to make sure that this does not happen in the future.

Spinners and progress bars however are a bit more challenging. They need to be refactored so that they are no long instantiated outside of the CLI code. They should instead be passed as a function parameter to each function that needs them. A good example of a project that solves the problem in a similar manner is Kind. In Kind they create a [status object from the logger](https://github.com/kubernetes-sigs/kind/blob/7799e72306db315ea4f4b1cac90ff68404da4f28/pkg/internal/cli/status.go#L39) which they then [pass to where it is needed](https://github.com/kubernetes-sigs/kind/blob/7799e72306db315ea4f4b1cac90ff68404da4f28/pkg/cluster/internal/create/create.go#L133). Doing so results in a single status object created which is reused where ever it is needed. A lot of inspiration can be take from how Kind delas with CLI output. While they use klog instead of slog there are a lot of similarities. They have for example a check if the output is in a terminal or not, and will disable the spinner accordingly.

## Consequences

Refactoring the message package would make importing Zarf packages as a library simpler. It would also simplify any unit testing and debugging efforts by using predictable errors. The main downside is that some detail from logs will be lost especially when moving the spinner code out of any packages. Giving the same amount of detailed messages would be very difficult. We should take care to preserve the critical information while also decreasing the verbosity of logs at certain locations.

0 comments on commit 3a9a87c

Please sign in to comment.