Skip to content

Conversation

corentinaltepe
Copy link
Contributor

@corentinaltepe corentinaltepe commented Apr 6, 2021

Fixes #113
Fixes #94
Fixes #95

This PR would allow solving #94 and #95 with the following call, although disabling logs is not the direct purpose of this PR:

_mongoRunner = MongoDbRunner.Start(logger: NullLogger.Instance);

How it works

When starting the mongod process, the ProcessControl evaluates if an ILogger has been injected or if it's null. If null, then MongoRunner works as usual, by wiring the logs to .NET's console and debug outputs. Otherwise, it parses mongo's logs and wires them to the ILogger.

Parsing mongo's logs is a discussable choice. I'm open for discussing this choice. I mainly needed to parse the json logs to get the severity so that we would call ILogger with the appropriate level.

Risks and Impacts

  • Added 2 new dependencies: Microsoft.Extensions.Logging.Abstractions for the ILogger interface, and System.Text.Json to parse the mongo logs.
  • Parsing the mongo logs totally depends on Mongo's definition of their logging structure. If they decide to break the format, we'll have to adapt. Notice an error in their documentation: they reported the id property to be a string, whereas it is actually an int.
  • This is slightly unrelated. I removed the argument string windowTitle from the ProcessControl.StartAndWait() methods, as I noticed they were unused. However, the class and the methods are public, meaning it could break usage if anyone has been using these methods. I believe the class ProcessControl should remain internal. Is that correct? Do we risk breaking anyone's usage with the changes I made here?

Performance

I ran this modified version of Mongo2Go on a test suite of 296 passing tests. I have not noticed any significant impact or improvement on execution time of the test suite. Duration kept between 02:00 and 2:20 (parallel), or 20-25 minutes cumulative. No significant change either caused by the logging level or the logging target.

@JohannesHoppe
Copy link
Member

Looks great so far!

string windowTitle ... This is super old and was apparently forgotten at some point.

@corentinaltepe corentinaltepe marked this pull request as ready for review April 6, 2021 14:57
@corentinaltepe
Copy link
Contributor Author

Thank you very much for your feedback. I believe the PR is ready for review now.

I have not updated the changelog. Should I, or is it something you should do when you next release?

@JohannesHoppe
Copy link
Member

JohannesHoppe commented Apr 6, 2021

updating the changelog

I appreciate any help! 😇

@corentinaltepe
Copy link
Contributor Author

I've added a new entry in the changelog. I'm not sure if this is how you intended to have it. Feel free to correct me.
Thanks again!

@JohannesHoppe JohannesHoppe merged commit d7bf673 into Mongo2Go:master Apr 7, 2021
@corentinaltepe corentinaltepe deleted the feature/logging branch April 7, 2021 21:34
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.

Configure logging Argument --quiet not working on azure devops Feature: Disable (optional) console output
2 participants