-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Stream logs to file using process.start. #227
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,84 @@ | ||||||
| import 'dart:io'; | ||||||
|
|
||||||
| /// Defines the available log levels. | ||||||
| class LogLevel { | ||||||
| const LogLevel._(this._level, this.name); | ||||||
|
|
||||||
| final int _level; | ||||||
|
|
||||||
| /// String name for the log level. | ||||||
| final String name; | ||||||
|
|
||||||
| /// LogLevel for messages instended for debugging. | ||||||
| static const LogLevel debug = LogLevel._(0, 'DEBUG'); | ||||||
|
|
||||||
| /// LogLevel for messages instended to provide information about the exection. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: typo in execution |
||||||
| static const LogLevel info = LogLevel._(1, 'INFO'); | ||||||
|
|
||||||
| /// LogLevel for messages instended to flag potential problems. | ||||||
| static const LogLevel warning = LogLevel._(2, 'WARN'); | ||||||
|
|
||||||
| /// LogLevel for errors in the execution. | ||||||
| static const LogLevel error = LogLevel._(3, 'ERROR'); | ||||||
| } | ||||||
|
|
||||||
| /// Abstract class for loggers. | ||||||
| abstract class Logger { | ||||||
| /// Processes a debug message. | ||||||
| void debug(Object message); | ||||||
|
|
||||||
| /// Processes an info message. | ||||||
| void info(Object message); | ||||||
|
|
||||||
| /// Processes a warning message. | ||||||
| void warning(Object message); | ||||||
|
|
||||||
| /// Processes an error message. | ||||||
| void error(Object message); | ||||||
| } | ||||||
|
|
||||||
| /// Logger to print message to standard output. | ||||||
| class PrintLogger implements Logger { | ||||||
| /// Creates a logger instance to print messages to standard output. | ||||||
| PrintLogger({ | ||||||
| IOSink out, | ||||||
| this.level = LogLevel.info, | ||||||
| }) : out = out ?? stdout; | ||||||
|
|
||||||
| /// The [IOSink] to print to. | ||||||
| final IOSink out; | ||||||
|
|
||||||
| /// Available log levels. | ||||||
| final LogLevel level; | ||||||
|
|
||||||
| @override | ||||||
| void debug(Object message) => _log(LogLevel.debug, message); | ||||||
|
|
||||||
| @override | ||||||
| void info(Object message) => _log(LogLevel.info, message); | ||||||
|
|
||||||
| @override | ||||||
| void warning(Object message) => _log(LogLevel.warning, message); | ||||||
|
|
||||||
| @override | ||||||
| void error(Object message) => _log(LogLevel.error, message); | ||||||
|
|
||||||
| void _log(LogLevel level, Object message) { | ||||||
| if (level._level >= this.level._level) | ||||||
| out.writeln(toLogString('$message', level: level)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Transforms a [message] with [level] to a string that contains the DateTime, | ||||||
| /// level and message. | ||||||
| String toLogString(String message, {LogLevel level}) { | ||||||
| final StringBuffer buffer = StringBuffer(); | ||||||
| buffer.write(DateTime.now().toIso8601String()); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
| buffer.write(': '); | ||||||
| if (level != null) { | ||||||
| buffer.write(level.name); | ||||||
| buffer.write(' '); | ||||||
| } | ||||||
| buffer.write(message); | ||||||
| return buffer.toString(); | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,12 @@ import 'dart:async'; | |
| import 'dart:convert'; | ||
| import 'dart:io'; | ||
|
|
||
| import 'package:file/file.dart'; | ||
| import 'package:file/local.dart'; | ||
| import 'package:file/memory.dart'; | ||
| import 'package:fuchsia_ctl/src/logger.dart'; | ||
| import 'package:meta/meta.dart'; | ||
| import 'package:pedantic/pedantic.dart'; | ||
| import 'package:process/process.dart'; | ||
|
|
||
| import 'operation_result.dart'; | ||
|
|
@@ -92,21 +97,84 @@ class SshClient { | |
| Future<OperationResult> runCommand(String targetIp, | ||
| {@required String identityFilePath, | ||
| @required List<String> command, | ||
| Duration timeoutMs = defaultSshTimeoutMs}) async { | ||
| Duration timeoutMs = defaultSshTimeoutMs, | ||
| String logFilePath, | ||
| FileSystem fs}) async { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: trailing comma for arg list
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| assert(targetIp != null); | ||
| assert(identityFilePath != null); | ||
| assert(command != null); | ||
|
|
||
| return OperationResult.fromProcessResult( | ||
| await processManager | ||
| .run( | ||
| getSshArguments( | ||
| identityFilePath: identityFilePath, | ||
| targetIp: targetIp, | ||
| command: command, | ||
| ), | ||
| ) | ||
| .timeout(timeoutMs), | ||
| final bool logToFile = !(logFilePath == null || logFilePath.isEmpty); | ||
| FileSystem fileSystem = fs ?? const LocalFileSystem(); | ||
| IOSink logFile; | ||
| Logger logger; | ||
|
|
||
| // If no file is passed to this method we create a memoryfile to keep to | ||
| // return the stdout in OperationResult. | ||
| if (logToFile) { | ||
| fileSystem.file(logFilePath).existsSync() ?? | ||
| fileSystem.file(logFilePath).deleteSync(); | ||
| fileSystem.file(logFilePath).createSync(); | ||
| final IOSink data = fileSystem.file(logFilePath).openWrite(); | ||
| logger = PrintLogger(out: data); | ||
| } else { | ||
| fileSystem = MemoryFileSystem(); | ||
| fileSystem.file('logs')..createSync(); | ||
| logFile = fileSystem.file('logs').openWrite(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like it's not used below and the output will just be an empty string if no log file is specified. I think we might be better off having some kind of Alternatively, if we're logging this to a file/stdout, do we really need to return stdout/stderr back to the caller?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed from here and moved the functionality to the logger class. |
||
| logger = PrintLogger(); | ||
| } | ||
|
|
||
| final Process process = await processManager.start( | ||
| getSshArguments( | ||
| identityFilePath: identityFilePath, | ||
| targetIp: targetIp, | ||
| command: command, | ||
| ), | ||
| ); | ||
| final StreamSubscription<String> stdoutSubscription = process.stdout | ||
| .transform(utf8.decoder) | ||
| .transform(const LineSplitter()) | ||
| .listen((String log) { | ||
| if (!logToFile) { | ||
| logFile.writeln(log); | ||
| } else { | ||
| logger.info(log); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we just do
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. It was implemented like that because logfile adds datetime and log level and all the ssh commands in fuchsia_ctl expects output with no additional prepended. |
||
| }); | ||
| final StreamSubscription<String> stderrSubscription = process.stderr | ||
| .transform(utf8.decoder) | ||
| .transform(const LineSplitter()) | ||
| .listen((String log) { | ||
| if (!logToFile) { | ||
| logFile.writeln(log); | ||
| } else { | ||
| logger.warning(log); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not error?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to error. |
||
| } | ||
| }); | ||
|
|
||
| // Wait for stdout and stderr to be fully processed because proc.exitCode | ||
| // may complete first. | ||
| await Future.wait<void>(<Future<void>>[ | ||
| stdoutSubscription.asFuture<void>(), | ||
| stderrSubscription.asFuture<void>(), | ||
| ]); | ||
| // The streams as futures have already completed, so waiting for the | ||
| // potentially async stream cancellation to complete likely has no benefit. | ||
| unawaited(stdoutSubscription.cancel()); | ||
| unawaited(stderrSubscription.cancel()); | ||
|
|
||
| final int exitCode = await process.exitCode.timeout(timeoutMs); | ||
|
|
||
| String output = ''; | ||
| if (!logToFile) { | ||
| logFile | ||
| ..flush() | ||
| ..close(); | ||
| output = await fileSystem.file('logs').readAsString(); | ||
| } | ||
|
|
||
| return exitCode != 0 | ||
| ? OperationResult.error('Failed', info: output) | ||
| : OperationResult.success(info: output); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,10 +14,11 @@ dependencies: | |
| file: ^5.0.10 | ||
| meta: ^1.1.7 | ||
| path: ^1.6.4 | ||
| pedantic: ^1.9.2 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please avoid having pedantic as a direct dependency - use it as a dev dependency instead. Although this package is not consumed by anyone, this can cause problems if we later need to use some other package that's also using pedantic with an incompatible constraint. Pedantic's semver constraints are really more about the analysis option rules anyway, rather than the We should just write our own
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| process: ^3.0.12 | ||
| retry: ^3.0.0+1 | ||
| uuid: ^2.0.2 | ||
|
|
||
| dev_dependencies: | ||
| test: ^1.6.9 | ||
| mockito: ^4.1.1 | ||
| test: ^1.6.9 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| // Copyright 2020 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| test('', () async {}); | ||
| } | ||
|
Comment on lines
+7
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe forgot to save before committing? :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep uploaded the version with the tests. |
||
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.
nit: trailing comma will make this format a bit more nicely
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.
done