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 local variables to notifications #123

Merged
merged 5 commits into from
Nov 11, 2022

Conversation

remstone7
Copy link
Contributor

#122

Adds local_variables to request

Copy link
Member

@joshuap joshuap left a comment

Choose a reason for hiding this comment

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

This is interesting—I didn't know this was possible in Python! Are there any performance implications or anything like that? There are in Ruby, which is one reason why we disable it by default. The other reason is security—the values could be sensitive, and I think reporting local variables is so advanced that it might surprise some people. For that reason I'd suggest we add a new config option similar to the Ruby one, and turn it off by default.

My other suggestion is that we apply the params_filters config option to the local_variables object so that any sensitive keys are filtered out.

@Kelvin4664 @subzero10 can you review when you get a sec?

@remstone7
Copy link
Contributor Author

@joshuap yup it has it in the inspect package here: https://docs.python.org/3/library/inspect.html - this is a built in so no performance impacts, this is similar to what sentry does, the only difference than how i implemented it is that they take the entire traceback, create a frame for each and then get the local variables for each frame (section of the traceback). other implementations like inspect.currentframe().f_locals

Makes sense about the sensitive keys and good call out here, will utilize param filters for that and then will also make it a configurable option 👍 thanks for the feedback!

@subzero10
Copy link
Member

LGTM. Thank you @remstone7!
I'll wait for the param filtering + config option before approving.

@remstone7
Copy link
Contributor Author

latest push:
using django to test a view but config:
image

inside the app:
image

@joshuap
Copy link
Member

joshuap commented Nov 10, 2022

Looking good here, well let @subzero10 review.

Since it's possible to get local variables for every frame of the stack trace, should we plan to modify our notice schema to add a local_variables option to the stack frame? We could then implement an expanded local_variables feature in a subsequent version of honeybadger-python.

@remstone7
Copy link
Contributor Author

This would be 🎉

Since it's possible to get local variables for every frame of the stack trace, should we plan to modify our notice schema to add a local_variables option to the stack frame? We could then implement an expanded local_variables feature in a subsequent version of honeybadger-python.

Copy link
Member

@subzero10 subzero10 left a comment

Choose a reason for hiding this comment

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

Great! I left a comment but it's not breaking, just a suggestion.

Can we do one last thing? Let's add this new configuration option to the README.md page.

@subzero10 subzero10 merged commit c375182 into honeybadger-io:master Nov 11, 2022
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