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 StormHandler for logging #58

Merged
merged 13 commits into from
Oct 31, 2014
Merged

Add StormHandler for logging #58

merged 13 commits into from
Oct 31, 2014

Conversation

dan-blanchard
Copy link
Member

I was updating a bunch of code to use Storm for logging instead of Python logging, and I was noticing a substantial performance degradation, since the logging level was only being checked on the Storm side.

This patch adds a StormHandler class that can be used to quickly update any Python code to send logging messages to Storm.

@dan-blanchard
Copy link
Member Author

I know logging is pretty boring stuff, but any thoughts on this?

@amontalenti
Copy link
Contributor

Oh, I missed this PR when it opened. Really like it, @dan-blanchard. I'll merge this soon.

@amontalenti
Copy link
Contributor

@msukmanowsky Can you give this a review?

@msukmanowsky
Copy link
Contributor

Big 👍 on this and falls under the "why didn't I think of that?" category. Thanks @dan-blanchard!

msukmanowsky added a commit that referenced this pull request Oct 31, 2014
@msukmanowsky msukmanowsky merged commit 7671efa into pystorm:master Oct 31, 2014
@dan-blanchard dan-blanchard deleted the add_storm_handler branch November 3, 2014 21:50
@dan-blanchard
Copy link
Member Author

Now I've just got to add the same thing to IO::Storm using log4perl.

@dan-blanchard
Copy link
Member Author

@codywilbourn I'm not sure if it was intentional, but you left this PR off the release notes for 1.1.0. It just seems like the kind of thing people might want to switch to using. 😄

@codywilbourn
Copy link
Contributor

Missed it looking over changes. Added to CHANGES.md as well as Github release notes.

@dan-blanchard
Copy link
Member Author

Thanks!

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.

4 participants