Skip to content

add means to capture locals only once#229

Closed
bucko909 wants to merge 1 commit into
getsentry:masterfrom
bucko909:master
Closed

add means to capture locals only once#229
bucko909 wants to merge 1 commit into
getsentry:masterfrom
bucko909:master

Conversation

@bucko909

@bucko909 bucko909 commented Jan 3, 2019

Copy link
Copy Markdown

with_locals can be very expensive to run, and we've found our software can get into trouble when exceptions with complex locals are triggered in a tight loop, however, the information contained in the locals is often extremely useful for debugging. This commit allows the application to specify a means of selecting if locals should be captured, with a sample method which captures only the first for a given traceback. The cost of hashing the stack is quite small compared to the cost of stringifying the locals. Clearly the code can leak quite badly if used in a project with code generation, but in general the set of tracebacks should be small.

Note this can be quite awkward as Sentry doesn't really have a means of searching for a capture with locals, though one can normally just hunt backwards for the first instance.

@bucko909

bucko909 commented Jan 3, 2019

Copy link
Copy Markdown
Author

I'm unclear how to test this (sorry, I've no experience of Travis), and I'm a bit unsure if I've really stuck the capture picker in the right place.

@bucko909

bucko909 commented Jan 3, 2019

Copy link
Copy Markdown
Author

(The code is based on our custom monkey patch against the legacy raven codebase, but I don't see that it should behave too differently.)

@codecov-io

codecov-io commented Jan 3, 2019

Copy link
Copy Markdown

Codecov Report

Merging #229 into master will decrease coverage by 1.81%.
The diff coverage is 24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
- Coverage   81.39%   79.58%   -1.82%     
==========================================
  Files          31       31              
  Lines        2591     2576      -15     
  Branches      408      404       -4     
==========================================
- Hits         2109     2050      -59     
- Misses        326      371      +45     
+ Partials      156      155       -1
Impacted Files Coverage Δ
sentry_sdk/utils.py 78% <24%> (-3.74%) ⬇️
sentry_sdk/integrations/aws_lambda.py 0% <0%> (-21%) ⬇️
sentry_sdk/debug.py 92.85% <0%> (-3.58%) ⬇️
sentry_sdk/integrations/django/transactions.py 87.93% <0%> (-3.45%) ⬇️
sentry_sdk/integrations/django/__init__.py 81.5% <0%> (-1.26%) ⬇️
sentry_sdk/integrations/argv.py 92.85% <0%> (-0.9%) ⬇️
sentry_sdk/scope.py 80.99% <0%> (-0.76%) ⬇️
sentry_sdk/integrations/modules.py 91.3% <0%> (-0.7%) ⬇️
sentry_sdk/integrations/excepthook.py 86.36% <0%> (-0.6%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update add1b87...8826117. Read the comment docs.

@bucko909

bucko909 commented Jan 3, 2019

Copy link
Copy Markdown
Author

Eventually I'll defeat the linting, but I don't understand the coverage errors (or the remaining test ones) as I've clearly not committed an extra 6% to the codebase.

@untitaker

Copy link
Copy Markdown
Member

I am closing this in favor of #247 which should allow you to skip repr() calls.

@untitaker untitaker closed this Feb 3, 2019
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