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

Increased logging detail on logged messages to include a date-time stamp #124

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ELadner
Copy link

@ELadner ELadner commented Sep 7, 2017

having multiple log files with non-timestamped entries makes correlating events between files difficult.

this change adds a simple [yyyy-mm-dd HH:MM:SS] timestamp at the beginning of each line logged to the various nwnx2 logfiles.

@virusman
Copy link
Member

virusman commented Sep 7, 2017

Thanks!
Could you make it configurable/optional? I'm not sure whether this should be the default mode, but I think it should be configurable. Verbose logs may grow to gigabytes, and with every line timestamped, this will happen even quicker.

@ELadner
Copy link
Author

ELadner commented Sep 7, 2017

Good point. I'll take a look at making it optional. I may require some additional guidance on integrating that with the INI file and associated infrastructure.

@ELadner
Copy link
Author

ELadner commented Sep 9, 2017

Added hooks into the INI system to enable or disable timestamps (default is no). One small issue I can't quite fix, though, is that it works correctly for every log except for the ODBC plugin log (which is where I really wanted it to work). I can't figure out why it's not honoring the logTimestamp setting from the INI file when the plugin is initialized. Thoughts?

@virusman
Copy link
Member

Are you sure you're updating the .so? ODBC is only special because it has to be renamed before you copy it to the NWN folder. That might be the cause.

@@ -114,6 +125,7 @@ class CNWNXBase
gline* nwnxConfig;
const char * confKey;
int debuglevel;
int logTimestamp;
Copy link
Member

Choose a reason for hiding this comment

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

Do you expect it to be anything other than true/false? (Like additional datetime formats)

echo "nwnx2 does not build on x64."
echo "Try running this in a 32bit chroot."
exit 1
export CC="gcc -m32"
Copy link
Member

Choose a reason for hiding this comment

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

This looks like an unrelated change

Copy link
Author

Choose a reason for hiding this comment

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

hmm.. can I exclude this change from the PR? or does it have to be reverted in my repo and re-committed?

@ELadner
Copy link
Author

ELadner commented Mar 21, 2018 via email

@virusman
Copy link
Member

I think it's better to store it as a bool then and use true/false.
Feel free to make commits on top of this branch, I'll just squash and merge them as a single changeset.

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.

2 participants