Use executing to infer code qualname

[alexmojaki]

This is one of the features mentioned in #748. As requested by @untitaker I've made executing an optional dependency.

[alexmojaki]

I'm getting a Django test failure, but when I try running the tests locally, I get Apps aren't loaded yet:

(venv) ➜  sentry-python git:(executing-qualname) export DJANGO_SETTINGS_MODULE=tests.integrations.django.myapp.settings
(venv) ➜  sentry-python git:(executing-qualname) python -m pytest -k django                                            
================================================================= test session starts ==================================================================
platform darwin -- Python 3.8.0, pytest-3.7.3, py-1.9.0, pluggy-0.13.1
rootdir: /Users/alexhall/Desktop/python/sentry-python, inifile: pytest.ini
plugins: celery-4.4.6, localserver-0.5.0, forked-1.1.3, cov-2.8.1
collected 415 items / 1 errors / 411 deselected / 7 skipped                                                                                            

======================================================================== ERRORS ========================================================================
_______________________________________________ ERROR collecting tests/integrations/django/test_basic.py _______________________________________________
tests/integrations/django/test_basic.py:8: in <module>
    from django.contrib.auth.models import User
venv/lib/python3.8/site-packages/django/contrib/auth/models.py:2: in <module>
    from django.contrib.auth.base_user import AbstractBaseUser, BaseUserManager
venv/lib/python3.8/site-packages/django/contrib/auth/base_user.py:47: in <module>
    class AbstractBaseUser(models.Model):
venv/lib/python3.8/site-packages/django/db/models/base.py:107: in __new__
    app_config = apps.get_containing_app_config(module)
venv/lib/python3.8/site-packages/django/apps/registry.py:252: in get_containing_app_config
    self.check_apps_ready()
venv/lib/python3.8/site-packages/django/apps/registry.py:135: in check_apps_ready
    raise AppRegistryNotReady("Apps aren't loaded yet.")
E   django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet.
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================================== 7 skipped, 411 deselected, 1 error in 3.62 seconds ==================================================

What's the procedure for testing without tox?

[alexmojaki]

I don't understand what's happening with the project coverage.

[untitaker]

@alexmojaki Hey, sorry for responding so late! Would it be possible to have this be behind some sort of feature flag or integration, even? I think one could use the global event processors similar to how gnu backtrace does it to overwrite the data. The reason I am asking is because I am still not sure if doing this sort of thing incurs performance overhead, and if so, I would hate to see it being enabled simply because some package is installed.

Now to your questions:

• Ignore code coverage, it's not set up properly
• the tests seem to pass on travis, so I think I was too late here. I am not sure what the issue was with running outside of tox?

[alexmojaki]

OK, I'm happy to make this optional based on an integration or something. However I don't know how to implement that. I can't take the approach of GNU backtrace because I need the actual code object (and similarly the other features need an actual frame object) which is not available in the data by the time I reach an event processor. Being able to use the frame directly within serialize_frame as I've done in this PR seems like the obvious (only?) route. But I don't know how to check for the integration in there. I tried:

def function_name(frame):
    # type: (FrameType) -> str
    from sentry_sdk import Hub
    from sentry_sdk.integrations.executing import ExecutingIntegration

    if Hub.current.get_integration(ExecutingIntegration) is None:
        return frame.f_code.co_name
    else:
        import executing
        return executing.Source.for_frame(frame).code_qualname(frame.f_code)

and apart from being awkward it doesn't work. I don't know how exactly Hub.current works, but apparently not like this. Any ideas?

[untitaker]

Ah sorry, I misremembered. The "prior art" can be found in the Django integration, where we access the hint parameter to get access to the actual exc_info. We then call walk_exception_chain to get a list of exc_infos, and the frame should then be available on the traceback object.

sentry-python/sentry_sdk/integrations/django/__init__.py

Lines 150 to 152 in dce439f

	for exception, (_, exc_value, _) in zip(
	reversed(values), walk_exception_chain(exc_info)
	):

I acknowledge this is extremely awkward, but it still kind of keeps the API surface between the core SDK and the other integrations minimal. If one had to fully separate the integrations from the core sdk the utility functions used could be easily vendored.

[untitaker]

I don't know how exactly Hub.current works, but apparently not like this.

It's a thread local. If the executing integration is enabled once it should at least be accessible on the same thread.

[alexmojaki]

Regarding performance, there are three main sources of overhead:

• executing.Source.for_frame(frame) (as used here to get the qualname) parses the source file and adds a bunch of metadata. I haven't measured this, but I think the cost is moderate. However, the object is cached based on the source filename, so each file is only parsed once. Once this is done, getting the qualname is a trivial lookup.
• Identifying the executing node (still coming) also has moderate overhead the first time, but again this is cached based on the instruction index. I actually have a test that 10,000 calls complete in under a second. The point is that the overhead of these features is proportional to the number of unique, distinct events that an app produces. If the same exception happens many times, that's not a problem, and sentry itself will be the main source of overhead.
• pure_eval is a different story. Its work is based on runtime values and doesn't leave much room for caching. And even if it was free, it's going to add more values to serialize and send. So I think this has the greatest potential for performance problems. On the other hand, it uses the same executing.Source.for_frame(frame), because it also needs to parse the file and use much of the same metadata. So if the PureEvalIntegration is active, even if the ExecutingIntegration isn't, I think it's reasonable to get the qualname anyway since there shouldn't be any performance problem.

[alexmojaki]

I don't know how exactly Hub.current works, but apparently not like this.

It's a thread local. If the executing integration is enabled once it should at least be accessible on the same thread.

What I found is that in test_attach_stacktrace_enabled, I have:

hub = Hub(...)
hub.capture_message("HI")

which leads directly to function_name where I check Hub.current which turns out to be different from hub above. I was hoping hub.capture_message would set Hub.current but at least where I need it to it doesn't.

[untitaker]

You're right. I would ignore this error for now, I would argue the testcase is unrealistic. Hub isn't supposed to be used directly, at least not like that. The testcases using sentry_init are more realistic.

I can fix that testcase up for you when the time comes.

[alexmojaki]

OK, this mostly works, but only if there's an actual exception. It doesn't work in cases like test_attach_stacktrace_enabled with attach_stacktrace=True and capture_message. I guess that's an acceptable limitation?

I don't think I can make the diff coverage 90% without jumping through unreasonable hoops.

[untitaker]

Looks great! Can you:

• open a PR against https://github.com/getsentry/sentry-docs to add documentation for this integration (closest contender for similar integration is GNU Backtrace, introduced in getsentry/sentry-docs@4382341)
• Add an entry like this to Sentry such that Sentry suggests people to enable this integration if executing is installed (when viewing an event)

[untitaker]

Merging this now so you can rebase the other prs