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

Add configuration directive to enable/disable MarkUs logging overall #63

Closed
benjaminvialle opened this Issue Sep 29, 2010 · 8 comments

Comments

Projects
None yet
3 participants
@benjaminvialle
Member

benjaminvialle commented Sep 29, 2010

As of MarkUs 0.6 we will be using MarkUsLogger. Apart from the time it takes to write the logs, MarkUs has to do some additional querying for some statements to log. It would be good if we could refactor this so that sys admins could disable MarkUs logging totally to avoid this small performance penalty.

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Apr 14, 2011

Member

This should be fairly easy to achieve. Config directive, then check if logging is turned on in MarkusLogger.

Member

jerboaa commented Apr 14, 2011

This should be fairly easy to achieve. Config directive, then check if logging is turned on in MarkusLogger.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Jan 19, 2013

Member

It doesn't look like this has been completed. I'll give it a shot.

Member

danielstjules commented Jan 19, 2013

It doesn't look like this has been completed. I'll give it a shot.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Jan 22, 2013

Member

I realize now that I probably should have asked another question before working on this. Please check the solution I posted in the pull request. I justified the logic in the pull request description.

If anyone feels that the additional LOC are unwarranted, given the priority of this feature, I'll just add a check in MarkusLogger.log() to prevent writing to the file.

Edit: Nevermind - I'll just do the check in MarkusLogger.log(). Further review shows that only a few log statements would actually benefit from these code changes, and that's not enough to justify the complication.

Member

danielstjules commented Jan 22, 2013

I realize now that I probably should have asked another question before working on this. Please check the solution I posted in the pull request. I justified the logic in the pull request description.

If anyone feels that the additional LOC are unwarranted, given the priority of this feature, I'll just add a check in MarkusLogger.log() to prevent writing to the file.

Edit: Nevermind - I'll just do the check in MarkusLogger.log(). Further review shows that only a few log statements would actually benefit from these code changes, and that's not enough to justify the complication.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Jan 22, 2013

Member

As detailed in the commit changes, setting MARKUS_LOGGING_ENABLED = false in config/environments/test.rb will cause MarkusLogger related Unit Tests to fail. I'm not sure what expected behaviour makes more sense. That they would fail, or that they would pass? If we'd rather they pass, I could just wrap them in a conditional block checking markus_config_logging_enabled? Would appreciate some input on this.

Member

danielstjules commented Jan 22, 2013

As detailed in the commit changes, setting MARKUS_LOGGING_ENABLED = false in config/environments/test.rb will cause MarkusLogger related Unit Tests to fail. I'm not sure what expected behaviour makes more sense. That they would fail, or that they would pass? If we'd rather they pass, I could just wrap them in a conditional block checking markus_config_logging_enabled? Would appreciate some input on this.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Jan 23, 2013

Member

@benjaminvialle Any thoughts on Pull Request #992 ?

Member

danielstjules commented Jan 23, 2013

@benjaminvialle Any thoughts on Pull Request #992 ?

@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@benjaminvialle

benjaminvialle Jan 23, 2013

Member

@danielstjules Sorry, I did not see your message in #992.
I think that what you suggest to do would "I could write tests that make sure nothing is written to the log after file creation" would be a good thing. And then avoid tests checking the logs.

Member

benjaminvialle commented Jan 23, 2013

@danielstjules Sorry, I did not see your message in #992.
I think that what you suggest to do would "I could write tests that make sure nothing is written to the log after file creation" would be a good thing. And then avoid tests checking the logs.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Jan 23, 2013

Member

Ok, I'll get that done sometime tonight.

Member

danielstjules commented Jan 23, 2013

Ok, I'll get that done sometime tonight.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Jan 23, 2013

Member

@benjaminvialle Here's the updated pull/commit. Let me know if you had anything else in mind for the fix.

Member

danielstjules commented Jan 23, 2013

@benjaminvialle Here's the updated pull/commit. Let me know if you had anything else in mind for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment