-
Notifications
You must be signed in to change notification settings - Fork 195
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
Conversation
if (isset($_SERVER['TERMINUS_LOG_DIR'])) { | ||
$logDirectory = $_SERVER['TERMINUS_LOG_DIR']; | ||
} elseif ($config['silent']) { | ||
$logDirectory = '/var/log/terminus/'; |
There was a problem hiding this comment.
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.
@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. |
73c8cde
to
d8207c9
Compare
Per @bensheldon's advice, I've updated the PR with implementation of BASH and JSON outputs. |
} elseif ($this->options['logFormat'] == 'bash') { | ||
$message = $this->formatBashMessages($level, $message, $context); | ||
} else { | ||
$message = $this->formatMessages($level, $message, $context); |
There was a problem hiding this comment.
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?
2d56f65
to
847f688
Compare
@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:
This PR sets the groundwork to allow all of these though so +1 |
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:
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. |
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". |
@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. |
847f688
to
bf1adc6
Compare
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? |
Closes #249 |
Looks like a reasonable PR, will get to it today |
I appreciate it very much, @katzgrau! |
Closes #199 |
bf1adc6
to
1ea3f2e
Compare
1ea3f2e
to
0bb0756
Compare
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. |
Switched Terminus logger for katzgrau/KLogger
Closes #208

Closes #260
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