[dotnet] Enabling drivers to set log to console. #16097
Conversation
This will be false by default, following what the .NET bindings have usually done. Helps with #12273
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
LogPath takes precedence anyway.
Messages will be shown to the console using the Logger functionality. Adding a test that listens to the logger and checks that logs are being sent to console. Fixing an issue when sending the log to a file, as the OnDriverProcessStarted method in the base class was not being invoked.
Internal variable to avoid using the stream when logs are sent to a file. Not exposed to the user.
| @@ -305,6 +317,16 @@ protected virtual void OnDriverProcessStarted(DriverProcessStartedEventArgs even | |||
| throw new ArgumentNullException(nameof(eventArgs), "eventArgs must not be null"); | |||
| } | |||
|
|
|||
| if (this.WriteDriverLogToConsole && eventArgs.StandardOutputStreamReader != null) | |||
| { | |||
| _ = Task.Run(() => ReadStreamAsync(eventArgs.StandardOutputStreamReader, "stdout")); | |||
There was a problem hiding this comment.
Don't use async Tasks to capture log messages. By the nature async is unpredictable when exactly it will be executed, meaning for us we will see messed log messages.
There was a problem hiding this comment.
Good suggestion. I wasn't aware of that in .NET.
There was a problem hiding this comment.
Just pushed changes to do sync threads instead.
| /// Collects the driver log output and writes it to the console. Defaults to <see langword="true"/>. | ||
| /// Internal variable to avoid using the stream when logs are sent to a file. | ||
| /// </summary> | ||
| protected bool WriteDriverLogToConsole { get; set; } = true; |
There was a problem hiding this comment.
I am pretty sure we don't need this property, despite its name is not clear. Just don't introduce it.
There was a problem hiding this comment.
In the case of sending GeckoDriver logs to a file, if we don't have a flag, even if it is internal, the streams get consumed by the console and the file. This ends up in an error because the console consumes the stream, and nothing gets sent to the file. That is why we need it.
Co-authored-by: Nikolay Borisenko <22616990+nvborisenko@users.noreply.github.com>
|
Please see minimal example how to capture output from process. Probably it helps. var service = new MyService();
service.Start();
class MyService
{
public void Start()
{
var process = new Process();
process.StartInfo.FileName = "ping";
process.StartInfo.Arguments = "google.com";
process.StartInfo.UseShellExecute = false;
process.StartInfo.CreateNoWindow = true;
process.StartInfo.RedirectStandardOutput = true;
process.StartInfo.RedirectStandardError = true;
process.OutputDataReceived += OnDataReceived;
process.ErrorDataReceived += OnDataReceived;
process.Start();
process.BeginOutputReadLine();
process.BeginErrorReadLine();
process.WaitForExit();
}
protected virtual void OnDataReceived(object sender, DataReceivedEventArgs args)
{
if (string.IsNullOrEmpty(args.Data))
return;
// This method can be used to handle data received from the process
Console.WriteLine($"Data received: {args.Data}");
}
} |
|
Thanks. I did try the approach and made changes to use the idea you suggest, but then I end up with errors like this one: Do you have any suggestions? I am only trying to log messages to console here, not even trying to log to a file yet. |
|
Please disregard the message above, I found out why this happens. |
User description
This will be false by default, following what the .NET bindings have usually done.
🔗 Related Issues
Helps with #12273
Should fix #15785
💥 What does this PR do?
It enables users to send driver logs to the console, disabled by default.
🔧 Implementation Notes
💡 Additional Considerations
I could only test it manually by creating a .NET app and using the build as a dependency.
@nvborisenko, could you please help me check this?
🔄 Types of changes
PR Type
Enhancement
Description
Add
LogToConsoleproperty toDriverServiceclassEnable driver process console output logging (disabled by default)
Handle mutual exclusivity between
LogToConsoleandLogPathin FirefoxDiagram Walkthrough
File Walkthrough
DriverService.cs
Add LogToConsole property and output redirectiondotnet/src/webdriver/DriverService.cs
LogToConsoleboolean property with XML documentationLogToConsoleis false
FirefoxDriverService.cs
Handle LogToConsole and LogPath mutual exclusivitydotnet/src/webdriver/Firefox/FirefoxDriverService.cs
LogToConsoleandLogPathLogToConsolewhenLogPathis specified