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

Separate Dredd's application logging from subprocess and reporter output #1089

Closed
honzajavorek opened this issue Jul 16, 2018 · 7 comments · Fixed by #1213
Closed

Separate Dredd's application logging from subprocess and reporter output #1089

honzajavorek opened this issue Jul 16, 2018 · 7 comments · Fixed by #1213
Labels
Epic: CLI reporter Dredd's default reporter released

Comments

@honzajavorek
Copy link
Contributor

Currently Dredd uses winston as it's main method of outputting to the command line. It has many flaws, primarily:

  • It combines reporters output and application logging
  • It outputs to both stdout and stderr inconsistently (certain levels go there, other ones go elsewhere) so it's difficult to parse or redirect
  • It pollutes Dredd's output so it's cluttered, ruins brievity of the dot reporter (Dredd redirects server output #593), the --names option, and other parts of Dredd
  • The --level option is inconvenient as it controls output of both application logging and reporter output
  • It makes debugging difficult
  • It blocks further development of reporters, namely Machine-readable reporter #341 and Redesign CLI reporter #765

The solution:

  • Use debug for Dredd application logging. Update docs, tests, and the .github/ISSUE_TEMPLATE.md file.
  • Use winston only in reporters. Replace it with process.stdout.write() later on and remove the --level option.
  • Take care of capturing the server/hooks output alongside the ideas in Dredd redirects server output #593 (comment) and let reporters to deal with it as they wish (probably a new event).
@ghost
Copy link

ghost commented Jul 16, 2018

@honzajavorek , I will be glad to implement the above functionality in dredd, with your approval. Can you please tell me about logging in reporters, so finally dredd won't be using winston at all?

@honzajavorek
Copy link
Contributor Author

@thesageinpilani I think this won't be easy to implement at all and will better be carried out by @michalholasek or me. I don't want to turn you down, I just do not expect this to be a nice experience and we could actually be able to figure it out ourselves.

I did review your previous PR and I'd be very grateful if you could work on that instead so we can fix all the issues mentioned there.

If this issue gets without addressing for 2 or 3 weeks, we failed to deliver the change on time and anyone is very welcome to contribute it if interested. But now I'd prefer if the maintainers could focus on the issue first.

@ghost
Copy link

ghost commented Jul 17, 2018

Thanks for the detailed review. I am grateful for guidance and support. 😀

@honzajavorek
Copy link
Contributor Author

@michalholasek one thought - looking at this it might be beneficial to keep the --level option or rename it to --debug and treat it as if setting DEBUG=dredd to simplify this for people.

The downside is that the code executed between $ dredd --debug and applying the setting (i.e. all code taking care of parsing CLI options etc.) won't be affected and won't output anything. It's the same case now - setting --level won't affect any code until actually set in the configuration, so you can't see any verbose etc. logging of the initial setup.

Might be still worth for all the cases where people do not need to debug the setup code. They wouldn't need to play with environment variables. What do you think?

@michalholasek
Copy link
Contributor

keep the --level option or rename it to --debug and treat it as if setting DEBUG=dredd to simplify this for people

@honzajavorek Yep, I agree.

@honzajavorek honzajavorek added the Epic: CLI reporter Dredd's default reporter label Jul 18, 2018
@michalholasek
Copy link
Contributor

michalholasek commented Jul 23, 2018

After discussion with @honzajavorek, we decided on the following:

A) DEBUG=dredd|* dredd

  • All levels combined (debug, info, warn(ing), error)
  • debug npm package

B) --loglevel|-l=debug|info|warn(ing)|error (renamed from --level)

  • Winston used only for reporter(s) output
  • Application (dredd) output
  • Levels:
    • debug (debug + info + warn + error)
    • info (info + warn + error)
    • warn (warn + error)
    • error

Loggging abstraction:

  • Independent from Winston (logger.js moved to reporters/logger.js)
  • debug -> debug package API
  • info -> process.stderr.write(...)
  • warn + error -> process.stderr.write(...)

michalholasek pushed a commit that referenced this issue Aug 2, 2018
BREAKING CHANGE: `--loglevel`'s default value is now 'error'. See #1089 for more info.a
michalholasek pushed a commit that referenced this issue Aug 2, 2018
BREAKING CHANGE: `--loglevel`'s default value is now 'error'. See #1089 for more info.
@honzajavorek honzajavorek modified the milestone: Improve CLI output Jan 12, 2019
honzajavorek added a commit that referenced this issue Jan 17, 2019
A part of the effort to separate application logging from the reporters output.

Addresses #1089, supersedes #1099, enables #765

BREAKING CHANGE: Instead of --level use --loglevel. The option is no longer able to affect reporter output, it only affects application logging output. Use --loglevel=silent instead of --silent, which is now removed. The --loglevel option now only accepts 'silent', 'error', 'warning', 'debug' as values.
honzajavorek added a commit that referenced this issue Jan 17, 2019
A part of the effort to separate application logging from the reporters output.

Enables #1089 and subsequently #765, supersedes #1099

BREAKING CHANGE: The --level option is no longer able to affect reporter output, it only affects application logging output.
honzajavorek added a commit that referenced this issue Jan 17, 2019
A part of the effort to separate application logging from the reporters output.

Addresses #1089, supersedes #1099, enables #765

BREAKING CHANGE: Instead of --level use --loglevel. The option is no longer able to affect reporter output, it only affects application logging output. Use --loglevel=silent instead of --silent, which is now removed. The --loglevel option now only accepts 'silent', 'error', 'warning', 'debug' as values.
honzajavorek added a commit that referenced this issue Jan 17, 2019
A part of the effort to separate application logging from the reporters output.

Enables #1089 and subsequently #765, supersedes #1099

BREAKING CHANGE: The --level option is no longer able to affect reporter output, it only affects application logging output.
honzajavorek added a commit that referenced this issue Jan 17, 2019
A part of the effort to separate application logging from the reporters output.

Addresses #1089, supersedes #1099, enables #765

BREAKING CHANGE: Instead of --level use --loglevel. The option is no longer able to affect reporter output, it only affects application logging output. Use --loglevel=silent instead of --silent, which is now removed. The --loglevel option now only accepts 'silent', 'error', 'warning', 'debug' as values.
honzajavorek added a commit that referenced this issue Jan 23, 2019
A part of the effort to separate application logging from the reporters output.

Enables #1089 and subsequently #765, supersedes #1099

BREAKING CHANGE: The --level option is no longer able to affect reporter output, it only affects application logging output.
honzajavorek added a commit that referenced this issue Jan 23, 2019
A part of the effort to separate application logging from the reporters output.

Addresses #1089, supersedes #1099, enables #765

BREAKING CHANGE: Instead of --level use --loglevel. The option is no longer able to affect reporter output, it only affects application logging output. Use --loglevel=silent instead of --silent, which is now removed. The --loglevel option now only accepts 'silent', 'error', 'warning', 'debug' as values.
honzajavorek added a commit that referenced this issue Jan 24, 2019
A part of the effort to separate application logging from the reporters output.

Addresses #1089, supersedes #1099, enables #765

BREAKING CHANGE: Instead of --level use --loglevel. The option is no longer able to affect reporter output, it only affects application logging output. Use --loglevel=silent instead of --silent, which is now removed. The --loglevel option now only accepts 'silent', 'error', 'warning', 'debug' as values.
honzajavorek added a commit that referenced this issue Feb 1, 2019
A part of the effort to separate application logging from the reporters output.

Enables #1089 and subsequently #765, supersedes #1099

BREAKING CHANGE: The --level option is no longer able to affect reporter output, it only affects application logging output.
honzajavorek added a commit that referenced this issue Feb 1, 2019
A part of the effort to separate application logging from the reporters output.

Addresses #1089, supersedes #1099, enables #765

BREAKING CHANGE: Instead of --level use --loglevel. The option is no longer able to affect reporter output, it only affects application logging output. Use --loglevel=silent instead of --silent, which is now removed. The --loglevel option now only accepts 'silent', 'error', 'warning', 'debug' as values.
@ApiaryBot
Copy link
Collaborator

🎉 This issue has been resolved in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic: CLI reporter Dredd's default reporter released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants