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

POC Tracing support #171

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pfreixes
Copy link
Contributor

@pfreixes pfreixes commented Jun 21, 2018

Implements a new tracing support for uvloop adding the pair methods
start_tracing and stop_tracing that allows the user to start or
stop the tracing. The user must provide a valid uvloop.tracing.Tracer
implementation that would be used internally by uvloop to create spans,
each span must also meet the uvloop.tracing.Span class contract.

This POC only implements the creation of spans during underlying calls
to the getaddrinfo function.

This POC relates to the conversation in #163.

What is missing/What is pending to discuss:

  • MetricsSpan, CounterSpan and TimingSpan controversial, this PR only implements a generic Span that is a timing one.

  • uvloop.tracing.Tracer.current_span needs to be adapted to avoid race conditions, when a span is created the "global" attribute is overwritten. Most likely current_span will end up as another context variable.

  • Agreement of an almost final draft of the start_tracing entry point, right now the implementation hides the TracedContext, so making it usable only internally by uvloop. I'm wondering if we must make it public providing to the user a way of creating spans in his code, so becoming the way of trace asyncio uvloop code.

  • Current implementation implicitly wires the Tracer context using the parent span, so for example when internally a new span is created using the TracedContext.start_span under the hood the parent span is passed to the Tracer.create_span, so allowing to the tracer to fetch some context information from this, also allowing make the new span child of the parent one. Should we reconsider this with a more explicit interface?

  • Support for other spans within uvloop. But before doing this, we would need a kind of agreement of behind what circumstances the spans are created. A good example is the create_connection loop function, would feel comfortable with something like this:

async def create_connection(...)
    with __traced_context() as span:
        .....

That it would mean that when there is no tracing enabled it will create a noop span, the benefits of this global wrapping is making the code cleaner and easier to maintain but the drawbacks are quite clear.

Also worth mentioning that going to a pattern that uses a global context would mean that we will be creating a span when indeed in some failure scenarios we connected to nothing and without yielding the loop.

Implements a new tracing support for uvloop adding the pair methods
`start_tracing` and `stop_tracing` that allows the user to start or
stop the tracing. The user must provide a valid `uvloop.tracing.Tracer`
implementation that would be used internally by uvloop to create spans,
each span must also meet the `uvloop.tracing.Span` class contract.

This POC only implements the creation of spans during underlying calls
to the `getaddrinfo` function.

This POC relates to the conversation in MagicStack#163.
__all__ = ('new_event_loop', 'EventLoopPolicy')
if PY37:
from .loop import start_tracing, stop_tracing
__all__ = ('new_event_loop', 'EventLoopPolicy', 'start_tracing', 'stop_tracing')
Copy link
Member

Choose a reason for hiding this comment

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

They should always be available, just raise NotImplementedError in 3.6/3.5.

else:
if not fut.cancelled():
fut.set_exception(result)

traced_context = __traced_context()
if traced_context:
Copy link
Member

Choose a reason for hiding this comment

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

Please use explicit is None checks.

@1st1
Copy link
Member

1st1 commented Jun 21, 2018

Looks good in general. I'll try to find time to get back to the discussion we have in the issue to see if we're missing anything.

@1st1
Copy link
Member

1st1 commented Jun 21, 2018

What is missing/What is pending to discuss:

This is a great summary, please update the issue with it.

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.

2 participants