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

Add log4net.xml, refactor logging init and log to file by default #1881

Merged
merged 7 commits into from Oct 8, 2016

Conversation

ayan4m1
Copy link
Contributor

@ayan4m1 ayan4m1 commented Sep 1, 2016

The logging initialization has been condensed into one class which will either load config from a log4net.xml in the app startup directory, or failing to find one tries to initialize the existing basic logging system. All of the logging entry points have been refactored to call this Logging.Initialize() method.

Tests are passing in the Windows/VS environment, review and feedback appreciated.

This would resolve #1452, which unfortunately has some project changes that are risky and needs a rebase.

@ayan4m1 ayan4m1 added the In progress We're still working on this label Sep 2, 2016
@ayan4m1 ayan4m1 self-assigned this Sep 3, 2016
@politas
Copy link
Member

politas commented Oct 8, 2016

So if I'm understanding this, by default this will not change anything, but if a log4net.xml exists, it'll use that, so we can tweak logging to get just the useful logging output for debugging purposes?

Copy link
Member

@politas politas left a comment

Choose a reason for hiding this comment

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

Builds fine, tests fine, runs fine. Code all looks good to me.

@politas politas added Merge-pending Ready for merging and removed In progress We're still working on this labels Oct 8, 2016
@politas politas merged commit a02ecbf into KSP-CKAN:master Oct 8, 2016
politas added a commit that referenced this pull request Oct 8, 2016
@politas politas removed Pull request Merge-pending Ready for merging labels Oct 8, 2016
@ayan4m1 ayan4m1 deleted the feature/file-logging branch October 8, 2016 13:22
@politas
Copy link
Member

politas commented Oct 9, 2016

Our Travis builds are failing since this was merged, on the "prove" step. It appears we are turning debug logging on by default somehow?

@ayan4m1
Copy link
Contributor Author

ayan4m1 commented Oct 10, 2016

Ah, I see why - because we are logging to the console during builds, there is more output from the "version" CLI command than just the version number. The test scripts are checking the output against a regex to make sure the version string looks correct, which it doesn't because there is more text being output than just the version string.

My thought is that the test is kinda weak but obviously the functionality has changed which may be undesirable. Let me know how you want to proceed.

@politas
Copy link
Member

politas commented Oct 11, 2016

Well, the trouble is that when you run any command, you get debugging level of logging, which shouldn't be the default.

$ ckan version execute
108 [1] DEBUG CKAN.CmdLine.MainClass (null) - CKAN started
192 [1] DEBUG CKAN.KSPManager (null) - Loading KSP instances from registry
198 [1] DEBUG CKAN.KSPManager (null) - Loading BuildID-bug from /home/politas/Desktop/KSP/KSP-1.1.2/BuildID-bug
200 [1] DEBUG CKAN.KSP (null) - Initialising /home/politas/Desktop/KSP/KSP-1.1.2/BuildID-bug/CKAN
201 [1] DEBUG CKAN.KSP (null) - Initialised /home/politas/Desktop/KSP/KSP-1.1.2/BuildID-bug/CKAN
208 [1] DEBUG CKAN.KSPManager (null) - Added BuildID-bug at /home/politas/Desktop/KSP/KSP-1.1.2/BuildID-bug

The command is supposed to return a line like this:

You are using CKAN version v1.20.1-30-g4e73422 (beta)

But all we get at the end of a bunch of debug lines is:

v1.20.1-30-g4e73422 (beta)

And the test is looking for the string "version"

I'm not sure why that is. I don't understand why it's doing that, and it shouldn't.

It isn't including the INFO level note "Debug logging enabled" that is inserted when the --debug option is detected, either.

The default logging fallback should not be DEBUG level

@ayan4m1
Copy link
Contributor Author

ayan4m1 commented Oct 16, 2016

I wrote the new log init to fallback if no log4net.xml was found in the current working directory, I'm thinking that is why some installs are DEBUG level by default. The log4net.xml isn't an embedded resource but I think it's possible to package it inside the CKAN binary and reference it from there. That way we are directly controlling the desired log level from inside the binary. I'll open a new PR if that sounds good to you.

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

Successfully merging this pull request may close these issues.

None yet

3 participants