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 a Twisted handler #69

Merged
merged 5 commits into from
Sep 23, 2015
Merged

Conversation

juggernaut
Copy link
Contributor

We have a couple of twisted services we want to report to rollbar. I figured it'd be useful to contribute if others are interested. It's cleaner to use twisted's async client instead of using the default 'thread' handler. I have modeled this after the Tornado handler. Please review. Thanks!

@brianr
Copy link
Member

brianr commented Sep 14, 2015

Thanks @juggernaut ! We'll review and get this merged.

self.remaining -= len(chunk)

def connectionLost(self, reason):
self.finished.callback(self.response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Rollbar requires an HTTPS connection, I would add in the following check for OpenSSL:

try:
    # Verify we can make HTTPS requests with Twisted.
    # From http://twistedmatrix.com/documents/12.0.0/core/howto/ssl.html
    from OpenSSL import SSL
except ImportError:
    log.exception('Rollbar requires SSL to work with Twisted')
    raise 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, done.

@juggernaut
Copy link
Contributor Author

Thanks for the review @coryvirok . I've made changes per your feedback.

@coryvirok
Copy link
Contributor

LGTM. Thanks!

coryvirok added a commit that referenced this pull request Sep 23, 2015
@coryvirok coryvirok merged commit f851c3d into rollbar:master Sep 23, 2015
@coryvirok
Copy link
Contributor

FYI - @juggernaut I found a couple of issues while doing more thorough tests for this. Changes here: #71

@juggernaut
Copy link
Contributor Author

thanks for fixing those @coryvirok

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.

3 participants