Re: Database "execute hooks" for instrumentation

[Shai Berger]

On Friday 15 September 2017 11:35:49 Shai Berger wrote:
> On Friday 15 September 2017 11:09:58 Anssi Kääriäinen wrote:
> > 
> > def simple_execute_hook(execute):
> > # One-time configuration and initialization.
> > 
> > def execute_hook(sql, params, many, context):
> > # Code to be executed for each cursor.execute() call.
> > # If many = True, the final call will be execute_many.
> > # The context parameter might contain stuff like used
> > # connection.
> > execute(sql, params, many, context)
> > # Code to be executed after the SQL has been ran.
> > 
> > return execute_hook
> > 
> > You would then add the hook with the connection's context manager.
> > 
> > The reason I'm asking is that this way the coding style would be
> > immediately familiar if you have used the request/response middlewares.
> 
> That's an interesting suggestion. At first look, it seems a nicer API than
> the context manager. I'm a little worried about how errors would be
> handled, though.

Well, of course, error handling was a red herring. It's up to the hook author.

But while looking deeper into it, I noted something else: The "One time 
configuration" part doesn't really make sense. I'll explain:

Since the `execute` argument is really a method on a cursor object which, 
potentially, doesn't even exist when the hook is installed, it needs to be 
passed into simple_execute_hook() near invocation time, rather than at 
registration time; and in fact, within the scope of one hook registration, we 
may need to handle separate cursors. So, the external function must be called 
again for every execution. Thus, the difference between code in the "one time 
configuration" (actually, "each time configuration") part and code in the "code 
to execute before query" part becomes arcane and hard to explain.

So, it becomes much more sensible to turn the hook into a wrapper, defined as:

def execute_wrapper(execute, sql, params, many, context):
# Code to be executed for each cursor.execute() call.
# If many = True, the final call will be execute_many.
# The context parameter might contain stuff like used
# connection.
result = execute(sql, params, many, context)
# Code to be executed after the SQL has been ran.
return result

Or even:

def execute_wrapper(execute, sql, params, many, context):
try:
# Code to be executed for each cursor.execute() call.
return execute(sql, params, many, context)
finally:
# Code to be executed after the SQL has been ran.

For this, I just need to figure out the currying of the execute parameter.

Shai

Re: Database "execute hooks" for instrumentation

[Shai Berger]

On Friday 15 September 2017 11:09:58 Anssi Kääriäinen wrote:
> Would it make sense to use the same technique used for HTTP
> request/response middleware? That is, the hook would look a bit like this:
> 
> def simple_execute_hook(execute):
> # One-time configuration and initialization.
> def execute_hook(sql, params, many, context):
> # Code to be executed for each cursor.execute() call.
> # If many = True, the final call will be execute_many.
> # The context parameter might contain stuff like used
> # connection.
> execute(sql, params, many, context)
> # Code to be executed after the SQL has been ran.
> return execute_hook
> 
> You would then add the hook with the connection's context manager.
> 
> The reason I'm asking is that this way the coding style would be
> immediately familiar if you have used the request/response middlewares.
> 

That's an interesting suggestion. At first look, it seems a nicer API than the 
context manager. I'm a little worried about how errors would be handled, 
though.

Re: Database "execute hooks" for instrumentation

[Anssi Kääriäinen]

Would it make sense to use the same technique used for HTTP request/response 
middleware? That is, the hook would look a bit like this:

def simple_execute_hook(execute):
# One-time configuration and initialization.
def execute_hook(sql, params, many, context):
# Code to be executed for each cursor.execute() call.
# If many = True, the final call will be execute_many.
# The context parameter might contain stuff like used
# connection.
execute(sql, params, many, context)
# Code to be executed after the SQL has been ran.
return execute_hook

You would then add the hook with the connection's context manager.

The reason I'm asking is that this way the coding style would be immediately 
familiar if you have used the request/response middlewares.

 - Anssi

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/e6098f14-0f0a-4ae6-b094-bfd9e2ccb51a%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Database "execute hooks" for instrumentation

[Shai Berger]

In case you're interested and want to see this in 2.0, please help:

https://code.djangoproject.com/ticket/28595
https://github.com/django/django/pull/9078

On Friday 14 April 2017 02:33:06 Adam Johnson wrote:
> django-perf-rec  would love this,
> it currently monkey patches connection.ops.last_executed_query to listen to
> all the queries going on.
> 
> On 7 April 2017 at 16:10, Shai Berger  wrote:
> > On Friday 07 April 2017 17:47:51 Carl Meyer wrote:
> > > Hi Shai,
> > > 
> > > On 04/07/2017 06:02 AM, Shai Berger wrote:
> > > > This is an idea that came up during the djangocon-europe conference:
> > Add
> > 
> > > > the ability to install general instrumentation hooks around the
> > 
> > database
> > 
> > > > "execute" and "executemany" calls.
> > > > 
> > > > Such hooks would allow all sorts of interesting features. For one,
> > > > they could replace the current special-case allowing
> > > > assertNumQueries & friends to record queries out of debug mode (it's
> > > > an ugly hack,
> > 
> > really),
> > 
> > > > but they could also support my imagined use case -- a context-manager
> > > > which could prevent database access during execution of some code
> > > > (I'm thinking mostly of using it around "render()" calls and
> > > > serialization, to make sure all database access is being done in the
> > > > view).
> > > 
> > > Another use-case is for preventing database access during tests unless
> > > specifically requested by the test (e.g. pytest-django does this,
> > > currently via monkeypatching).
> > 
> > Yep. This feels right.
> > 
> > > > My idea for implementation is to keep a thread-local stack of
> > > > context- managers, and have them wrap each call of "execute". We
> > > > could actually even use one such context-manager instead of the
> > > > existing
> > > > CursorDebugWrapper.
> > > 
> > > Why a new thread-local instead of explicitly per-connection and stored
> > > on the connection?
> > 
> > Sorry for implying that it would be a new thread-local, I just hadn't
> > thought
> > it through when I wrote this. Of course it goes on the (already
> > thread-local)
> > connection.
> > 
> > Shai.

Re: Database "execute hooks" for instrumentation

[Adam Johnson]

django-perf-rec  would love this,
it currently monkey patches connection.ops.last_executed_query to listen to
all the queries going on.

On 7 April 2017 at 16:10, Shai Berger  wrote:

> On Friday 07 April 2017 17:47:51 Carl Meyer wrote:
> > Hi Shai,
> >
> > On 04/07/2017 06:02 AM, Shai Berger wrote:
> > > This is an idea that came up during the djangocon-europe conference:
> Add
> > > the ability to install general instrumentation hooks around the
> database
> > > "execute" and "executemany" calls.
> > >
> > > Such hooks would allow all sorts of interesting features. For one, they
> > > could replace the current special-case allowing assertNumQueries &
> > > friends to record queries out of debug mode (it's an ugly hack,
> really),
> > > but they could also support my imagined use case -- a context-manager
> > > which could prevent database access during execution of some code (I'm
> > > thinking mostly of using it around "render()" calls and serialization,
> > > to make sure all database access is being done in the view).
> >
> > Another use-case is for preventing database access during tests unless
> > specifically requested by the test (e.g. pytest-django does this,
> > currently via monkeypatching).
> >
>
> Yep. This feels right.
>
> > > My idea for implementation is to keep a thread-local stack of context-
> > > managers, and have them wrap each call of "execute". We could actually
> > > even use one such context-manager instead of the existing
> > > CursorDebugWrapper.
> >
> > Why a new thread-local instead of explicitly per-connection and stored
> > on the connection?
> >
>
> Sorry for implying that it would be a new thread-local, I just hadn't
> thought
> it through when I wrote this. Of course it goes on the (already
> thread-local)
> connection.
>
> Shai.
>



-- 
Adam

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAMyDDM00XnYSh0rNQ91-ALG2toCoT3RCgyBqpsFvq%2BpSccx5PA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: Database "execute hooks" for instrumentation

[Shai Berger]

On Friday 07 April 2017 17:47:51 Carl Meyer wrote:
> Hi Shai,
> 
> On 04/07/2017 06:02 AM, Shai Berger wrote:
> > This is an idea that came up during the djangocon-europe conference: Add
> > the ability to install general instrumentation hooks around the database
> > "execute" and "executemany" calls.
> > 
> > Such hooks would allow all sorts of interesting features. For one, they
> > could replace the current special-case allowing assertNumQueries &
> > friends to record queries out of debug mode (it's an ugly hack, really),
> > but they could also support my imagined use case -- a context-manager
> > which could prevent database access during execution of some code (I'm
> > thinking mostly of using it around "render()" calls and serialization,
> > to make sure all database access is being done in the view).
> 
> Another use-case is for preventing database access during tests unless
> specifically requested by the test (e.g. pytest-django does this,
> currently via monkeypatching).
> 

Yep. This feels right.

> > My idea for implementation is to keep a thread-local stack of context-
> > managers, and have them wrap each call of "execute". We could actually
> > even use one such context-manager instead of the existing
> > CursorDebugWrapper.
> 
> Why a new thread-local instead of explicitly per-connection and stored
> on the connection?
> 

Sorry for implying that it would be a new thread-local, I just hadn't thought 
it through when I wrote this. Of course it goes on the (already thread-local) 
connection.

Shai.

Re: Database "execute hooks" for instrumentation

[Carl Meyer]

Hi Shai,

On 04/07/2017 06:02 AM, Shai Berger wrote:
> This is an idea that came up during the djangocon-europe conference: Add the 
> ability to install general instrumentation hooks around the database 
> "execute" 
> and "executemany" calls.
> 
> Such hooks would allow all sorts of interesting features. For one, they could 
> replace the current special-case allowing assertNumQueries & friends to 
> record 
> queries out of debug mode (it's an ugly hack, really), but they could also 
> support my imagined use case -- a context-manager which could prevent 
> database 
> access during execution of some code (I'm thinking mostly of using it around 
> "render()" calls and serialization, to make sure all database access is being 
> done in the view).

Another use-case is for preventing database access during tests unless
specifically requested by the test (e.g. pytest-django does this,
currently via monkeypatching).

> My idea for implementation is to keep a thread-local stack of context-
> managers, and have them wrap each call of "execute". We could actually even 
> use one such context-manager instead of the existing CursorDebugWrapper.

Why a new thread-local instead of explicitly per-connection and stored
on the connection?

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/5d5ee8f1-6b22-c48c-7a6b-11eac2928b64%40oddbird.net.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature