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

Verbose Simulation Output #295

Closed
wants to merge 1 commit into from

Conversation

EasterTheBunny
Copy link
Contributor

By default, the simulation output was already verbose by printing logs for each node to separate files and printing simulation output to a file.

This change makes verbose no longer default and adds an option to do verbose logging output.

By default, the simulation output was already verbose by printing logs for
each node to separate files and printing simulation output to a file.

This change makes verbose no longer default and adds an option to do verbose
logging output.
@@ -26,6 +26,7 @@ import (
var (
simulationFile = flag.StringP("simulation-file", "f", "./simulation_plan.json", "file path to read simulation config from")
outputDirectory = flag.StringP("output-directory", "o", "./simulation_plan_logs", "directory path to output log files")
verbose = flag.BoolP("verbose", "v", false, "make output verbose (prints logs to output directory)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about having 2 flags here:

  1. log level (info/debug/warn/error/none)
  2. log output (, "." for outputDirectory or none for CLI)
  • i.e. to have the output directory as a base dir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding level adds a good bit of scope because then one would need to decide what to log at each level. I think for now, to keep it simple, it should just be verbose (development logs) and not verbose (CI output).

The default directory is ./simulation_plan_logs right now to make sure the output does not get included in git commits. This directory is included in .gitignore to help developers keep commits clean of any accidents. We could change it to the base directory, but then we need to update .gitignore as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Our development logs are too informative so it might be hard to read, could have been better to have at least the level to let developers run with info or warn to get minimal output.

Regarding the the output, if we only enable to print logs to file system, it would be hard to understand issues in CI.
By printing to stdout we able to run anywhere while still being able to read the logs

@EasterTheBunny EasterTheBunny deleted the AUTO-6932/verbose-simulation-output branch December 13, 2023 18:58
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.

2 participants