itamarst / eliot

Eliot: the logging system that tells you *why* it happened
https://eliot.readthedocs.io
Apache License 2.0
1.1k stars 66 forks source link

Add context-preserving decorator for generators #375

Closed exarkun closed 5 years ago

exarkun commented 5 years ago

Fixes #259

In addition to a decorator for plain old generators, this also introduces the tiny extra helper necessary to get Twisted's inlineCallbacks playing nicely with Eliot. This takes the form of a new decorator that just applies both inlineCallbacks and the generator decorator being introduced.

Suggestions for better names for any of this welcome. Let me know if there's any other structural work to be done (docs or whatever).

This work made possible by https://leastauthority.com/

exarkun commented 5 years ago

Here is a silly little microbenchmark that I wrote to evaluate a couple different implementations against each other. I wasn't able to discern any difference with it, so maybe it's invalid or there was no meaningful performance difference in my candidates.

def benchmark():
    from eliot import start_action

    def measure(label, f):
        from time import clock
        g = f(1000)
        start = clock()
        set(g)
        end = clock()
        print("{} seconds: {}".format(label, end - start))

    @eliot_friendly_generator_function
    def friendly(baseline, depth, n):
        if depth == 0:
            for i in xrange(n):
                with start_action(action_type=u"foo"):
                    if not baseline:
                        yield
        else:
            with start_action(action_type=u"nesting"):
                set(friendly(baseline, depth - 1, n))

    measure("baseline", lambda n: friendly(True, 15, n))
    measure("decorated", lambda n: friendly(False, 15, n))

if __name__ == '__main__':
    benchmark()
itamarst commented 5 years ago

Probably want documentation for regular generators too. Might take a stab at that as part of verifying the semantics make sense to me.

itamarst commented 5 years ago

Just figured out how to use magit with github, I think, so will try to make some local changes and push them to your fork/branch. Which might or might not work? I may need to ask you to give me write access to the fork.

exarkun commented 5 years ago

Just figured out how to use magit with github, I think, so will try to make some local changes and push them to your fork/branch. Which might or might not work? I may need to ask you to give me write access to the fork.

I thiiiink it will just work. Because of that checkbox that defaults to checked that I probably didn't fiddle with. But, annoyingly, it seems like github doesn't tell you if you left it checked or not? Anyway, let me know if you have trouble.

itamarst commented 5 years ago

Question 1: What should we do about regular generators?

After playing around with some code, I feel like the behavior when you don't decorate it with eliot_friendly_generator is actually closer to what you'd expect: the context at time of iteration is what is used. With the decorator you get context at time of construction. However, with start_action(): yield is completely broken.

(For generator-as-pseudo-coroutine the decorator behavior does make sense.)

So perhaps for generators in general this could be solved by documentation ("don't use start_action() around a yield"). Telling people "you must use this decorator on generators" is also asking them to remember something, so I'm not sure the cognitive load is any different.

Thoughts?

itamarst commented 5 years ago

Question 2: Can we get rid of use_generator_context? Possibly we can have the decorator automatically run it?

itamarst commented 5 years ago

Todo: I would like an API to set the sub-context-factory, and for it to blow up if you set it more than once.

I can imagine this might cause problems for someone somewhere who wants to use Twisted inlineCallbacks in same program as asyncio and Eliot, but given Eliot userbase is small enough that's a rare use-case we can punt until that's really an issue.

exarkun commented 5 years ago

FWIW, I don't currently have any use-cases involving regular generators. I'm sure they're out there I just haven't bumped into them yet. The codebase I'm working in uses inlineCallbacks a lot and other forms of generators not so much.

itamarst commented 5 years ago

Yeah, I realize this is for inlineCallbacks only, but generators are code people use too, and this is effectively a possible solution for that, so want to figure out what to do while we're at it.

itamarst commented 5 years ago

OK, so I guess for this to be merged I'd like:

  1. Internal API for setting sub-context-factory, which complains if it's called twice with different factories.
  2. @eliot_friendly_generator_etc. should probably be private, and it should automatically set the sub-context-factory, unless that turns out to be too tricky (since typically this is done at import time I'm not too worried about performance... but might need a lock.)
  3. Documentation for generators that basically says "don't do yield inside with start_action."

If you'd rather I do the above let me know.

itamarst commented 5 years ago

Merging to non-master branch in main repo for continued development.