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

Consider using Django 2.0 execute_wrapper() #214

Open
shaib opened this issue Sep 23, 2017 · 3 comments · May be fixed by #631
Open

Consider using Django 2.0 execute_wrapper() #214

shaib opened this issue Sep 23, 2017 · 3 comments · May be fixed by #631

Comments

@shaib
Copy link

shaib commented Sep 23, 2017

Silk currently uses monkeypatching to install its wrapper around SQLCompiler.execute_sql. Django 2.0 provides a new API for installing wrappers around SQL executions, and it may be preferable to use this API when available.

Points arising:

  • The new API works at a lower level, that of the connection rather than SQLCompiler. I'm not sure what the implications are.
  • The new API installs the wrapper only in the current thread and in a context-manager which removes it on exit, these seem to be improvements over the current monkey-patching.
  • There is an intention to make the new API available for older versions of Django, via a 3rd-party package. So, when checking for its availability, the right check is not a Django version check, but rather hasattr(connection, 'execute_wrapper').
@avelis
Copy link
Collaborator

avelis commented Oct 5, 2017

@shaib If you have the time I encourage you to make a PR with this enhancement. I can take a look and merge it in.

@SebCorbin
Copy link
Contributor

SebCorbin commented Nov 20, 2022

Coming from #630

Issue

I've tracked down the issue to SQLInsertCompiler not being patched to log queries. IMHO patching compilers is dangerous, because all have a different behaviors, and some call super().execute_sql(), some don't (like SQLInsertCompiler). Hence I looked into our possibilities:

Method 1: Keep patching

Keep patching the compilers, I've tried to patch all 5 compilers:

  • SQLCompiler
  • SQLInsertCompiler
  • SQLDeleteCompiler
  • SQLUpdateCompiler
  • SQLAggregateCompiler

but this leads to either recursion errors (because of a super() call), or some dirty patching, along with possibly altering the behavior of the user's compilers (reminder: compiler depends on DATABASES['ENGINE'] which can be 3rd-party, e.g. Oracle or Postgis).
Also, I suspect our way of patching being the root cause of these issues:

Method 2: Wrap the cursor, not the compiler

I took a look at how django-debug-toolbar logs requests, this is less intrusive than patching compilers. We loose the access to query and query.model to differentiate Silky queries from the rest, but it can be replaced by simple string search on tables.

Method 3: Add a db wrapper on the connection for the request

As @shaib mentioned, it is possible to wrap cleanly (since Django 2.0) SQL queries, through a context manager, in the middleware for example:

class SilkyMiddleware:
    def __init__(self, get_response):
        self.get_response = get_response

    def __call__(self, request):
        self.process_request(request)

        query_logger = SilkyQueryLogger()
        with connection.execute_wrapper(qquery_logger):
            response = self.get_response(request)

        return self.process_response(request, response)

This is the cleanest way to log queries, and supported through official API. As in method 2, we don't have access to query model context.

Method 4: Add a permanent db wrapper on the connection in app.ready()

This is slightly different from method 3, because here the wrapper is added on app.ready(), to be made permanent, giving an advantage we might want to solve some feature requests like:

This should work:

class SilkAppConfig(AppConfig):
    default_auto_field = "django.db.models.AutoField"
    name = "silk"


    def ready(self):
        # Add wrapper to db connection
        if not any(isinstance(wrapper, SilkQueryWrapper) for wrapper in connection.execute_wrappers):
            connection.execute_wrappers.append(SilkQueryWrapper())

I've prepared a pull request for that last method, @jazzband/django-silk what's your take on this?

SebCorbin added a commit that referenced this issue Nov 20, 2022
@SebCorbin SebCorbin linked a pull request Nov 20, 2022 that will close this issue
@SebCorbin SebCorbin linked a pull request Nov 20, 2022 that will close this issue
@OscarVanL
Copy link

OscarVanL commented Mar 23, 2024

This would be awesome. I have several celery jobs, management commands, and other non-API contexts where I'd love to get the data I get from django-silk.

As a temporary workaround, I found this GitHub gist that you can use to run django-silk against arbitrary functions not wrapped in APIs. It would be fantastic to see something supported by django-silk in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants