Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom logger for unstructured and structured logging #171

Merged
merged 30 commits into from
Jul 14, 2023

Conversation

anishnaik
Copy link
Collaborator

@anishnaik anishnaik commented Jun 27, 2023

This PR closes #19 and introduces a custom logging API that has the capabilities to support both structured and unstructured logging. The logging API works as follows:

logging.Info("Hello", colors.Red, "World", colors.Reset, 1234, StructuredLogInfo{"key": "value"})

The API effectively accepts a variadic set of arguments. These arguments are evaluated one by one and are dealt with accordingly:

  1. If you are a "color" (e.g. colors.Red) then the next n arguments are colored with that color until we hit a colors.Reset.
  2. If you are a StructuredLogInfo then that info becomes a set of key-value pairs for that log object.
  3. Otherwise, you are just fmt.Sprintf("%v").

The API also allows you to create sub-loggers. Thus, there is one GlobalLogger that is instantiated in NewFuzzer and each "module" (e.g. fuzzer, corpus) can create a sub-logger that can be used to contextualize each module's log message with their own "module" key-value pair.

Each Logger consists of a consoleLogger and a multiLogger. The consoleLogger is used for console output whereas the multiLogger holds a list of writers that can perform both structured or unstructured logging to an arbitrary channel. This can be useful for third-party integrations down the line. Currently, at setup the GlobalLogger is enabled with console logging and unstructured logging to one file (if requested by the config). Then, the fuzzer creates its own sub-logger off of that. The downside of this approach is that if you AddWriter to the GlobalLogger, then any sub-loggers branched off of it do not have access to the new writer. This downside should be evaluated during this PR review. Note that due to this design, the Fatal log level is not supported. This is because it is not possible to log to both channels before hitting an os.Exit(1).

We also introduce the idea of a LogBuffer in this PR that allows you to create complex, formatted strings for log output (e.g. execution tracer). A LogBuffer should ideally be used only right before logging or if a String() capability is required. Otherwise, even an []any is sufficient for logging.

The following should be evaluated before merging this PR:

  • There are no unit tests for this PR. The compiled binary should be run on a few examples and the console (and file) output should be evaluated.
  • Does the current capability of AddWriter / RemoveWriter deter any future integrations. Is the use of a GlobalLogger the best way to approach this? How do we handle adding new writers to a variety of different sub-loggers?
  • Is the color scheme too "colorful"?
  • Are there any instances where we need to add more logging? Maybe less?
  • Is the API flexible enough to support our various use cases?
  • Are there any cases of crappy / clunky code that can be improved?

@anishnaik anishnaik requested a review from Xenomega as a code owner June 27, 2023 21:11
@anishnaik anishnaik changed the title WIP: Custom logger for unstructured and structured logging Custom logger for unstructured and structured logging Jul 6, 2023
fuzzing/fuzzer.go Outdated Show resolved Hide resolved
logging/log_buffer.go Outdated Show resolved Hide resolved
@Xenomega
Copy link
Member

Xenomega commented Jul 11, 2023

So I'm well through this review. Generally, I think it looks pretty great.

My thoughts so far is:

  • I don't love the manual construction of StructuredLogInfo { "error": <err> } each time, so it'd be nice to wrap that somehow so a single call can be easier written without being as prone to error (mistyping "error"), etc.
    • In other frameworks, you often can just do logger.Error(err), but I get that isn't great here for coloration.
    • It'd be nice to use StructuredLogInfo for "additional fields" and in the case of logger.Error(), it'd be nice to have it supplied without providing additional fields optionally. Maybe like supporting the following cases (1) log a string, (2) log an err, (3) log a string and an error (and append as you normally would), (4) support structured log info for additional info.
  • Otherwise, the only other thing I still need to review, which I can give feedback on later, is the actual JSON output. I don't think we output this based on comments in the config.go file, if so, that's fine and we can plan this out later.
    • I think I'd just support the following cases later:
      • Have base fields every log contains (message, level, date/time, and maybe an error field that's nil/empty for non-error messages).
      • Have additional fields populated in a field in our JSON output called "additionalInfo" or "details" or something.

Hopefully that makes sense. I think if we can cook up a good solution to the first major bullet point, I'd generally be good to merge here.

@anishnaik
Copy link
Collaborator Author

anishnaik commented Jul 11, 2023

The PR has been updated as follows:

  1. The API also now provides the ability to pass an error directly as part of the variadic list of arguments. This allows for easier use of the API instead of throwing the error into the StructuredLogInfo object. Note that only 1 error is supported per log message
  2. The StructuredLogInfo object is no longer represented as a list of key-value pairs in the log output but instead as a single JSON dictionary.
    e.g.
    Before: {"msg": "message", "key": value", "key2": "value2"}
    After: {"msg": "message", "info": {"key": "value", "key2": "value2"}
  3. Errors can be outputted at any log level - so even a trace log can have an error attached to it
  4. The "fuzz:..." messages have become colorized

Xenomega and others added 4 commits July 12, 2023 21:08
- Remove `CyanBold` and use `Bold` instead for coloring because it looks bad
- Remove default " " delimiter and added spaces wherever necessary.
- Update windows kernel calls to enable ANSI escape codes on stderr as well
- Add an `init()` function to `colors` package so that ANSI escape codes are enabled during package import
-  Some small update to formatting
Copy link
Member

@Xenomega Xenomega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bing bong

@anishnaik anishnaik merged commit 25b5de9 into master Jul 14, 2023
8 checks passed
@anishnaik anishnaik deleted the dev/multi-logger branch July 14, 2023 16:06
damilolaedwards pushed a commit that referenced this pull request Nov 16, 2023
This PR introduces colorized logging for console and future support for structured logging
---------

Co-authored-by: David Pokora <dpokora@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor logging to make use of structured logging providers
2 participants