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

Added functionality to capture logs from Plebian and log them in Monarch #114

Merged

Conversation

pawelvds
Copy link
Contributor

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

{Summary of the changes}

Description

{Detail}

Fixes #{issue number}

@pawelvds pawelvds linked an issue Oct 20, 2023 that may be closed by this pull request
@pawelvds pawelvds self-assigned this Oct 20, 2023
@pawelvds pawelvds requested a review from volllly October 23, 2023 07:50
@pawelvds pawelvds marked this pull request as ready for review October 23, 2023 10:27
@pawelvds pawelvds requested a review from a team as a code owner October 23, 2023 10:27
Copy link
Contributor Author

@pawelvds pawelvds left a comment

Choose a reason for hiding this comment

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

In the last commit version of the ProcessHostMonarch class, I implemented a cleaner and more modern coding style, along with improvements to logging and error handling.

Process initialisation: The process is now initialised using a more concise and readable object initialiser. The change from _process = new Process(); to _process = new Process { StartInfo = new ProcessStartInfo } is more in line with modern C# practices, making the code cleaner and easier to follow.

Buffered output and error streams: The standard output and error streams are now explicitly set to buffer. This ensures that we capture all output for analysis.
When messages are logged from the buffer, the buffer is cleared to avoid logging the same messages again if the process is restarted.

@pawelvds pawelvds added feature-request New feature or request planned-feature A feature that was planned and committed to by the maintainers labels Oct 23, 2023
@volllly
Copy link
Contributor

volllly commented Oct 24, 2023

Very sorry to do that but I've noticed while testing that the whole restart mechanism did not work correctly (the restart actually failed and we did not actually set the error code) so I've revamped it and Fixed #116 in the process 😅

I've tried to implement your suggestions from #116 do you have any more ideas for clarifying the code?

Copy link
Contributor Author

@pawelvds pawelvds left a comment

Choose a reason for hiding this comment

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

Awesome changes, great job

@volllly volllly merged commit 93a7773 into main Oct 25, 2023
15 checks passed
@volllly volllly deleted the 97-improve-logging-on-plebian-startup-in-case-of-a-crash branch October 25, 2023 07:40
@github-actions github-actions bot locked and limited conversation to collaborators Oct 25, 2023
@volllly volllly linked an issue Oct 25, 2023 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request New feature or request planned-feature A feature that was planned and committed to by the maintainers
Projects
None yet
2 participants