Skip to content

[core] provide a wrap_executor#221

Merged
palazzem merged 4 commits into
masterfrom
palazzem/wrap-executor
Mar 31, 2017
Merged

[core] provide a wrap_executor#221
palazzem merged 4 commits into
masterfrom
palazzem/wrap-executor

Conversation

@palazzem

@palazzem palazzem commented Mar 13, 2017

Copy link
Copy Markdown

What it does

Adds a wrap_executor callback to the Tracer.configure() method, so that a contrib module can change the behavior of Tracer.wrap(). This is required because in some frameworks (i.e. Tornado), the Tracer.wrap() must handle "coroutine" executions that returns immediately a Future instance. On the other hand, we can't pollute the Tracer.wrap() with contrib code and PR goals are:

  • if a wrap_executor is not defined, the Tracer.wrap() behaves as usual for regular functions or asyncio coroutines
  • if a wrap_executor is defined, decorated functions are handled using this callable

Usage

from ddtrace import tracer

# this is the default signature
def wrap_executor(tracer, fn, args, kwargs, span_name, service, resource, span_type):
    with tracer.trace(span_name, service=service, resource=resource, span_type=span_type):
        # do something before
        result = fn(*args, **kwargs)
        # do something after
        return result

# enable the custom decorator handler
tracer.configure(wrap_executor=wrap_executor)

Integration example

Look at the Tornado integration and usage.

Limitation

The wrap_executor is executed each time the decorator is called. We can't use a wrap_factory that returns a new decorator because it could be too late. Indeed, users can decorate their functions before calling the tracer.configure() method. This may add a little overhead (order of nanoseconds) because the following is always evaluated for each call:

if getattr(self, '_wrap_executor', None):
    # _wrap_executor call

@palazzem palazzem added the core label Mar 13, 2017
@palazzem palazzem added this to the 0.7.0 milestone Mar 13, 2017
@palazzem
palazzem requested a review from LotharSee March 13, 2017 09:31
@palazzem
palazzem force-pushed the palazzem/wrap-executor branch from 4bc41e3 to cd07a05 Compare March 13, 2017 09:45
@palazzem
palazzem requested a review from LeoCavaille March 13, 2017 10:30
@palazzem palazzem modified the milestones: 0.7.0, 0.8.0 Mar 29, 2017
@palazzem
palazzem force-pushed the palazzem/wrap-executor branch from cd07a05 to 8f50372 Compare March 30, 2017 13:16
Comment thread ddtrace/tracer.py
span_name,
service=service,
resource=resource,
span_type=span_type,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the default signature for wrap_executor, are service, resource, span_type kwargs or args? This usage, and the one in your test are inconsistent.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread tests/test_tracer.py
eq_(writer.spans[0].name, 'wrap.overwrite')
eq_(writer.spans[0].get_tag('args'), '(42,)')
eq_(writer.spans[0].get_tag('kwargs'), '{\'kw_param\': 42}')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we expand to this to check a nested case i.e. wrapped_function by another traced function -> check if service is correctly propagated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread ddtrace/tracer.py
:param object context_provider: The ``ContextProvider`` that will be used to retrieve
automatically the current call context

:param object wrap_executor: callable that is used when a function is decorated with

@LotharSee LotharSee Mar 30, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a comment that context_provider/wrap_executor are advanced options that the user should not modify.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment like: This is an advanced option that usually doesn't need to be changed from the default value

@palazzem
palazzem merged commit 7c745c9 into master Mar 31, 2017
@palazzem
palazzem deleted the palazzem/wrap-executor branch March 31, 2017 11:49
@palazzem

Copy link
Copy Markdown
Author

@LotharSee merged because this one doesn't introduce any incompatibility changes

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

Successfully merging this pull request may close these issues.

3 participants