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

Switched Terminus logger for katzgrau/KLogger #477

Merged
merged 1 commit into from
Sep 15, 2015
Merged

Conversation

TeslaDethray
Copy link
Contributor

Closes #208
Closes #260
image

I found a logger which elegantly extends PSR's logger, and even had the same function names. It accepts an output destination, which effectively silences the logs whenever they are unwanted. What I've done here is:
-Exchanged the Terminus Logger for katzgrau's KLogger.
-Renamed the old logger to outputter and switched the things it still needs to handle output on to use the outputter instead
-Made properties in TerminusCommand for both logger and outputter, which is how they'd be called in the future.

If accepted, in the future:
-Get rid of TerminusCommand::logtype('message', array()) and use $this->logger->type('message') and $this->outputter->success/failure('message'), eliminating the redundant vsprintf commands in TerminusCommand.
-Create an outputter to replace the other outputting calls

if (isset($_SERVER['TERMINUS_LOG_DIR'])) {
$logDirectory = $_SERVER['TERMINUS_LOG_DIR'];
} elseif ($config['silent']) {
$logDirectory = '/var/log/terminus/';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't imagine log files are in the same place on Windows, so this probably has to change.

@TeslaDethray
Copy link
Contributor Author

@ronan I was trying out a couple of open-source loggers, and this one worked so well that it was done before I knew it. I'm very interested in your feedback on this.

@TeslaDethray
Copy link
Contributor Author

Per @bensheldon's advice, I've updated the PR with implementation of BASH and JSON outputs.
image

} elseif ($this->options['logFormat'] == 'bash') {
$message = $this->formatBashMessages($level, $message, $context);
} else {
$message = $this->formatMessages($level, $message, $context);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left all these parameters in so that these functions' signatures would match the original version. I am not sure whether it is smarter to have it match for consistency and clarity, or to pass in a $parts array. Thoughts?

@TeslaDethray TeslaDethray force-pushed the new_logger branch 4 times, most recently from 2d56f65 to 847f688 Compare September 11, 2015 23:46
@ronan
Copy link
Contributor

ronan commented Sep 14, 2015

@TeslaDethray +1 on using a well supported OSS logger (which implements PSR-3). BIG +1 on separating logging from output.

I would suggest as a followup to this:

  • Ensure that the logger writes to STDERR or a file (never to STDOUT)
  • Ensure that all command results (and only results) go to the outputter to be sent to STDOUT
  • Ensure that each command has a single, consistent output (ie: list sites should always return an array even it if is empty, the 'no sites' message should get logged)
  • --silent should only affect logging, not output. (ie: Remove the 'quiet' outputter)
  • Allow outputters or output formatters to be swapped out based on the requested format
  • Nice to have: Wrangle the output of drush and wp-cli so that we can send them to the outputter to be formatted consistently

This PR sets the groundwork to allow all of these though so +1

@bensheldon
Copy link
Contributor

The situation that isn't mentioned is "Interactive Output".

I think it's maybe confusing because I'm using Logger to mean "The mechanism by which we print things to the console buffer", and not "the thing we use to persist telemetry and debug information for inspection".

The 3 circumstances when we output things to the console buffer are:

  1. Interactive commands (which both involve asking for feedback, but also having a dialogue with the user about what is happening and why)
  2. Terminating output (here is your list of sites)
  3. Telemetry/debug data

In this PR, I think we're just discussing the mechanism for outputting things to the console buffer (better description than that?) and not really discussing how to best handle telemetry/debug data as a traditional "logger" would describe.

...so I'm fine with this PR as is and I think there should be a higher level architecture meeting to discuss all of the circumstances for rendering/persisting info.

@bensheldon
Copy link
Contributor

Also, I should admit that I think a lot of the confusion/challenge has to do with supporting both interactive modes and command flags at the same time which was part of the original specification. I'm not sure if this is the time to re-open that discussion or just say "it's a sticky wicket".

@ronan
Copy link
Contributor

ronan commented Sep 14, 2015

@bensheldon I agree with your breakdown of the three circumstances but I disagree with what this PR helps with. In my view the KLogger helps with telemetry/debug data. It could be used to handle terminating output but it is not well suited for that.

I think we need an "outputter" concept as well whose only job is to take a single piece of output data and format it in whatever way is appropriate (JSON, ASCII table, Awk/Sed friendly text etc.). Ideally each command would return a value to the runner which would format the output and print it to STDOUT. Commands would not have direct access to write to the console. That would ensure that they're not leaking info and debug messages into the data that may be piped to another command. Logging (debug, info and errors) would go through the logger object which would format it and write it to a file, send it to STDERR or both.

If an unrecoverable error occurred (no response from the server, user not logged in) the command should throw an exception which is caught by the runner which logs the error and exits with an error code.

I don't have a strong opinion on how interactive I/O should go but I assume it would be most appropriate to send prompts to STDOUT since the requirement that STDOUT have clean output is somewhat moot in interactive mode. OTOH: Prompting via STDERR could allow the user to interact with Terminus even while piping the output. Not sure if that's the case though.

The above would be a reasonably large refactoring so a more appropriate baby step would be to have commands call the 'outputter' as needed similar to how they call the logger currently. This is not as clean conceptually but requires less work. This outputter would replace the previous logger and compliment the new logger.

I also agree with the confusion regarding interactive mode and command flags but if we make sure our output is clean (ie: only the results go to STDOUT) then the only risk is if we add a new option to a command and prompt for it if it is missing, then updates to Terminus could break existing scripts.

@TeslaDethray
Copy link
Contributor Author

I updated the logger to target stderr by default (Thanks, @ronan!) and added a new issue (#249) for fixing sites list, which is out of scope for this pull request.

I made a pull request against the logger's repo to enable targeting STDERR. (katzgrau/KLogger#66) I'm not sure how long it may take Katzgrau to respond, though. How do we usually handle needing these things included?

@TeslaDethray
Copy link
Contributor Author

Closes #249

@katzgrau
Copy link

Looks like a reasonable PR, will get to it today

@TeslaDethray
Copy link
Contributor Author

I appreciate it very much, @katzgrau!

@TeslaDethray
Copy link
Contributor Author

Closes #199

@TeslaDethray
Copy link
Contributor Author

Since we need the logger change in an unreasonable amount of time (today), I've copied it into this repository and created a ticket (#484) for replacing it with Katzgrau/KLogger after.

TeslaDethray added a commit that referenced this pull request Sep 15, 2015
Switched Terminus logger for katzgrau/KLogger
@TeslaDethray TeslaDethray merged commit f2dbec1 into master Sep 15, 2015
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.

None yet

4 participants