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

No instrument / transaction decorators for async def #633

Closed
bitdivision opened this issue Mar 11, 2021 · 5 comments
Closed

No instrument / transaction decorators for async def #633

bitdivision opened this issue Mar 11, 2021 · 5 comments

Comments

@bitdivision
Copy link

Currently, the instrument and transaction decorators inherit from ContextDecorator which does not support decorating async def functions. Would be good to get an implementation which supports both.

@tim-schilling
Copy link
Collaborator

Hi @bitdivision,

I'm focused on #469 currently, but suspect this issue will be addressed as a part of that or soon after.

Thanks,
Tim

@bitdivision
Copy link
Author

Just as an addendum, I found that using @instrument(<some function name>) all the time was a bit painful. If the API for this is going to change then I think it would be useful to default the operation argument to f"{func.__module__}.{func.__qualname__}" or similar, meaning that you can just use @instrument() without specifying the function name.

If it helps, I currently have the following decorator, which supports async / sync and defaults the operation unless it is specified. (This uses a get_tracked_request function which will need to be changed if used in the library. Also I don't think it'll be python2 compatible.)

class function_trace:  # pylint: disable=invalid-name
    """Decorator to allow instrumenting both async and sync functions.

    By default will use `Custom/<module_name>.<function.__qualname__` as the span operation name
    """

    def __init__(self, manual_operation=None, kind="Custom", tags=None):
        self.manual_operation = manual_operation
        self.kind = kind
        if tags is None:
            self.tags = {}
        else:
            self.tags = tags
        self.span = None
        self.result = None
        self.tracked_request = None

    def _start(self, func):
        self.tracked_request = get_tracked_request()
        qualified_name = self.manual_operation or f"{func.__module__}.{func.__qualname__}"
        operation_name = f"{text(self.kind)}/{qualified_name}"

        span = self.tracked_request.start_span(operation=operation_name)
        for key, value in self.tags.items():
            span.tag(key, value)

    def _finish(self):
        self.tracked_request.stop_span()

    def __call__(self, fn):
        if asyncio.iscoroutinefunction(fn):

            async def wrap(*args, **kwargs):
                self._start(fn)
                self.result = await fn(*args, *kwargs)
                self._finish()
                return self.result

        else:

            def wrap(*args, **kwargs):
                self._start(fn)
                self.result = fn(*args, *kwargs)
                self._finish()
                return self.result

        return wrap

@tim-schilling
Copy link
Collaborator

@bitdivision do you have an example of the decorator not working properly for an async function?

@bitdivision
Copy link
Author

I don't have a specific example @tim-schilling but I had assumed it was not working properly for all async functions. I've forgotten the exact failure mode I was seeing.

There's a python bug around this functionality where you have a sync ContextDecorator and want to decorate an async function: https://bugs.python.org/issue37398

@tim-schilling
Copy link
Collaborator

Not a problem. I was able to break things and create a fix.

I'm less inclined to try to use asyncio.iscoroutinefunction for the same reasons listed in the python ticket. It's possible for iscoroutinefunction to return false when the function returns an awaitable.

The new API will be something like:

@instrument.async_("Foo")
async def foo():

tim-schilling added a commit to tim-schilling/scout_apm_python that referenced this issue Apr 6, 2021
tim-schilling added a commit to tim-schilling/scout_apm_python that referenced this issue Apr 6, 2021
tim-schilling added a commit to tim-schilling/scout_apm_python that referenced this issue Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants