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 additional hooks on django middleware #139

Merged
merged 1 commit into from
Feb 16, 2018
Merged

Add additional hooks on django middleware #139

merged 1 commit into from
Feb 16, 2018

Conversation

filwaitman
Copy link
Contributor

Child of #137 (see this for the entire thread)

@ezarowny
Copy link
Contributor

Ah, I see what you might want to do here. I'm going to discuss this with another engineer. Could be useful!

@ezarowny
Copy link
Contributor

ezarowny commented Nov 9, 2016

Hey there, just wanted to say that we haven't forgotten about you. We want to support this feature and we're trying to decide the best way to go about it.

@filwaitman
Copy link
Contributor Author

hahaha

ok @ezarowny, no rush! =]

@ezarowny
Copy link
Contributor

ezarowny commented Dec 15, 2016

Hey @filwaitman, sorry for the delay there.

The middleware solution you presented is definitely a valid way to go about this but I've been trying to think of a way that would feel more official. As far as I know, you can't pass any arguments to the __init__(...) function of a Django Middleware.

As Django's settings files are just Python files, what do you think about storing or importing the get_payload_data and get_extra_data functions into your Django settings file? We could then look for them when initializing the Middleware?

I'm not sure if this is actually any simpler yet. Just an idea I've been mulling over.

@filwaitman
Copy link
Contributor Author

Hi!

Sounds a good idea.
I can't work on it now, though. I guess next month I can do that.

@ezarowny
Copy link
Contributor

Oh that's alright. I'll get a PR together with how I think this should work and I'll tag you so we can discuss it.

@filwaitman
Copy link
Contributor Author

Ok, sounds perfect!

@coryvirok
Copy link
Contributor

@ezarowny what's next for this PR?

@ezarowny
Copy link
Contributor

Just need to write up the handlers for Django and test it out.

@GabLeRoux
Copy link
Contributor

I'm interested in this as well, any progress or plan to support this? I'm looking for a way to specify which user had the problem with some stacktrace and this seems like a potential solution.

Copy link
Contributor

@GabLeRoux GabLeRoux left a comment

Choose a reason for hiding this comment

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

I tested this pull request on top of latest master branch and I confirm it works as expected. I see custom keys in params on the rollbar dashboard 👍

image

Any plan to merge this? :shipit:

Example I used:

from rollbar.contrib.django.middleware import RollbarNotifierMiddleware

class CustomRollbarNotifierMiddleware(RollbarNotifierMiddleware):

    @staticmethod
    def get_extra_data(request, exc):
        try:
            # returning user here is kind of pointless as it's already included in `person`
            # but still, this is a valid example
            return dict({'user': request.user})
        except Exception:
            # if things go wrong, don't make it worst and let the rollbar notification leave
            return None

Copy link
Contributor

@rokob rokob left a comment

Choose a reason for hiding this comment

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

This looks good as is to me

@rokob rokob merged commit 4954e6b into rollbar:master Feb 16, 2018
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

6 participants