strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
https://strawberry.rocks/docs/django
MIT License
421 stars 122 forks source link

Strawberry breaks django debug toolbar #586

Closed ldynia closed 4 months ago

ldynia commented 4 months ago

Strawberry brakes the latest django-debug-toolbar 4.4.6

Describe the Bug

File "/home/nonroot/.local/lib/python3.11/site-packages/strawberry_django/middlewares/debug_toolbar.py", line 11, in <module>
    from debug_toolbar.middleware import _HTML_TYPES, get_show_toolbar  # noqa: PLC2701
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ImportError: cannot import name '_HTML_TYPES' from 'debug_toolbar.middleware' (/home/nonroot/.local/lib/python3.11/site-packages/debug_toolbar/middleware.py)
Process SpawnProcess-1:
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/multiprocessing/process.py", line 314, in _bootstrap
    self.run()
  File "/usr/local/lib/python3.11/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "/home/nonroot/.local/lib/python3.11/site-packages/uvicorn/_subprocess.py", line 80, in subprocess_started
    target(sockets=sockets)
  File "/home/nonroot/.local/lib/python3.11/site-packages/uvicorn/server.py", line 65, in run
    return asyncio.run(self.serve(sockets=sockets))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "uvloop/loop.pyx", line 1517, in uvloop.loop.Loop.run_until_complete
  File "/home/nonroot/.local/lib/python3.11/site-packages/uvicorn/server.py", line 69, in serve
    await self._serve(sockets)
  File "/home/nonroot/.local/lib/python3.11/site-packages/uvicorn/server.py", line 76, in _serve
    config.load()
  File "/home/nonroot/.local/lib/python3.11/site-packages/uvicorn/config.py", line 434, in load
    self.loaded_app = import_from_string(self.app)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nonroot/.local/lib/python3.11/site-packages/uvicorn/importer.py", line 19, in import_from_string
    module = importlib.import_module(module_str)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1204, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1176, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1147, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 690, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 940, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/usr/src/config/asgi.py", line 19, in <module>
    application_django = get_asgi_application()

System Information

Additional Context

Parallel issue opened with django toolbar issue

Upvote & Fund

Fund with Polar

bellini666 commented 4 months ago

Hrmm, they removed _HTML_TYPES from the code.

Also, it seems like they now have builtin support for async, which means we can probably remove a lot of the workaround to make async work from here.

We should bump the requirement to the latest version and refactor our wrapper

matthiask commented 4 months ago

@bellini666 _HTML_TYPES still exists but it has been moved to a different place: https://github.com/jazzband/django-debug-toolbar/commit/982a1271c452ae382c5a851a5484ed3056ef47be

is_processable_html_response is not documented either but at least it looks a little bit less internal. Maybe you could take advantage of that.

The async stuff is brewing, and much could change in the following weeks. I have high hopes!

bellini666 commented 4 months ago

The async stuff is brewing, and much could change in the following weeks. I have high hopes!

Oh, good to know! Do you have any links to any internal discussions which I can follow?

Because I'm wondering if I should just adapt the current code to import _HTML_TYPES from that other place, and then do any adjustments required after the the async stuff has settled down.

tim-schilling commented 4 months ago

Do you have any links to any internal discussions which I can follow?

For now, those discussions can be followed in our meeting notes via Google Docs. I added you as a viewer just now.

bellini666 commented 4 months ago

Hey @tim-schilling ,

Thanks for sharing the docs!

Btw, taking a quick look at it I noticed that there's an issue about SQL not being instrumented for async. When looking at the current implementation, just a callout that you might need to change panel.enable_instrumentation() to await sync_to_async(panel.enable_instrumentation)() in https://github.com/jazzband/django-debug-toolbar/blob/main/debug_toolbar/middleware.py#L115 to make it work

That is what I did in this lib to support SQL instrumentation for both WSGI and ASGI (https://github.com/strawberry-graphql/strawberry-django/blob/main/strawberry_django/middlewares/debug_toolbar.py#L107-L128), but of course in a much more ugly way as I had to monkey patch a lot =P

salty-ivy commented 4 months ago

panel.enable_instrumentation() to await sync_to_async(panel.enable_instrumentation)() in That is what I did in this lib to support SQL instrumentation for both WSGI and ASGI

@bellini666 Could you also explain how you came to this conclusion or why it is working ( perhaps in the google docs ), I am currently investigating more solid ways of making SQLPanel async compatible.

(https://github.com/strawberry-graphql/strawberry-django/blob/main/strawberry_django/middlewares/debug_toolbar.py#L107-L128), but of course in a much

I wonder if this only works with SQLite and has any performance header ? as this would probably run everything in same thread one after another including the DB connections that are supposed to form in separate threads at times.

more info here https://github.com/django/asgiref/blob/29c4d92f6a8fc9bf1df03abd4aa4874158c6ea27/asgiref/sync.py#L344

bellini666 commented 4 months ago

@bellini666 Could you also explain how you came to this conclusion or why it is working ( perhaps in the google docs ), I am currently investigating more solid ways of making SQLPanel async compatible.

Sure! When running any async code, there will be a single thread responsible for running any DB related code, which runs when doing sync_to_async(some_fund, thread_sensisive=True).

When enabling the instrumentation, the panel will iterate over the connections and patch them, meaning that running it directly will patch connections in the wrong thread. By wrapping it with sync_to_async(thread_sensitive=True) you ensure that you are patching the connections from the thread_sensitive thread, which are the ones that are actually going to be used to run any DB queries.

Let me know if you this explanation is enough or you need more details.

I can also add that to the doc but I don't have permission to edit it

bellini666 commented 4 months ago

I wonder if this only works with SQLite and has any performance header ? as this would probably run everything in same thread one after another including the DB connections that are supposed to form in separate threads at times.

It should work for everything. I have used this to instrument SQL for both SQLite and PostgreSQL in both WSGI/ASGI modes, even when using uvicorn, and it was always working correctly

salty-ivy commented 4 months ago

@bellini666 Thanks a lot , this saved me a lot of reading and investigation.

bellini666 commented 4 months ago

@salty-ivy this is going to be closed after I merge https://github.com/strawberry-graphql/strawberry-django/pull/600, but I'll keep an eye on newer releases of the debug toolbar to refactor this library's code later :)