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

Enhanced tracing middleware #21

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@SEJeff

SEJeff commented Apr 25, 2018

This PR does three things:

  • Fixes various PEP8 errors
  • Supercedes #5 after fixing conflicts
  • Adds extra information on the Spans created using Opentracing's server-side tags
  • Creates the infrastructure for custom hooks to be called when a Span is created or finished

philipgian and others added some commits Feb 2, 2017

Add opentracing server-side tags
Add opentracing-defined server-side tags to spans created by this
middleware. These tags add information regarding the origin of the
span and also denote that the span was created by a server. This kind of
information can later be used by the tracer (e.g. the jaeger-client adds
Zipkin compatible 'ss', 'sr' annotations based on this information).
Add support for tracing hooks
Add hooks to be called when a trace starts or finishes. This acts as an
integration point to interact with the spans handled by this Django
middleware.

The hooks are python callables and they are provided by the Django
settings in the form of a dictionary:

  {'start': start_hook, 'finish': finish_hook}

The start hook expects the newly created span as argument.
The finish_hook expects the span that is ready to be finished.

Any of them can be omitted.
Do not handle the span if a hook decides so
Add support to defer managing the spans to an external entity decided by
the provided hooks. A hook can return a 'True' value and let the
middleware know that it should not handle the Span.

- If the 'start_hook' returns true, then the middleware should not keep
  any reference to the Span.

- If the 'finish_hook' returns true, then no further handling is needed
  for the span in hands.
  Note that based on the return value of the start hook, a reference to
  a Span for the given request may not even exist, but the finish hook
  must be called with a `None` argument.

This allows of better integration with a Span manager and enables
advanced scenarios (e.g. the Span manager accumulates and report spans
in batches for performance reasons).
Merge remote-tracking branch 'philipgian/master' into enhanced-tracin…
…g-middleware

Manually resolved merge conflicts

@SEJeff SEJeff referenced this pull request Apr 25, 2018

Closed

Enhance tracing middleware #5

@SEJeff

This comment has been minimized.

SEJeff commented Apr 25, 2018

Tests are in progress and I'll push them when they're working. It will however, be a bit tricky without the MockTracer from opentracing/opentracing-python#3 in the 2.x branch of opentracing and the span.tags dict along with tracer.finished_spans(). Primarily because opentracing.tracing.Tracer doesn't set any state for operation_name, tags, logs, etc, so there is no way to assert they've been set.

I'd planned on asserting tags were set in the start and finish hooks to show they work. How do you want this tested @bhs? Again, once opentracing-python 2.x is released this will be super easy (and pleasant) to test.

return span
# add span to current spans
self._current_spans[request] = span

This comment has been minimized.

@bhs

bhs Apr 25, 2018

Contributor

Is it possible to "leak" map keys here? I.e., is there any chance (error paths, etc) for _finish_tracing to not-be-called?

This comment has been minimized.

@SEJeff

SEJeff Apr 25, 2018

Yes. When I was fixing the merge conflicts I kind of looked at that specific line. It does seem like it should be above the if start_hook and callable(start_hook) and start_hook(span) conditional, but I was just updating things as it was. Also, this could potentially have problems which I've addressed with #20. However, I'm going to adopt the same tag names for #20 as they do in opentracing-python here and will likely put most of the code inside trace.py and not inside the middleware. Should be able to get both of those done tonight. What do you think?

OPENTRACING_HOOKS = {'start': start_hook, 'finish': finish_hook}
- The start hook expects the newly created span as argument.

This comment has been minimized.

@bhs

bhs Apr 25, 2018

Contributor

start_hook

This comment has been minimized.

@SEJeff

SEJeff May 13, 2018

@bhs are you sure about that? The name of the hook is start, and the callable is start_hook. Your comment seems incorrect to me.

This comment has been minimized.

@bhs

bhs May 14, 2018

Contributor

@SEJeff I think you're right – I was reading this too quickly.

Note that based on the return value of the start hook, a reference to
a Span for the given request may not even exist, but the finish hook
will be called with a `None` argument.

This comment has been minimized.

@carlosalberto

carlosalberto Apr 26, 2018

Collaborator

Personally I think returning False for both hooks is the one that should be causing the Span to not be handled. Also, I honestly think that, if we don't handle the Span for start_hook, we should do some extra effort and not invoke the finish_hook at all.

That being said: I remember more than a few people asking for these kind of hooks, so this is definitely something nice to have - however, I don't remember anybody wanting to discard Span instances from these.

This comment has been minimized.

@philipgian

philipgian Apr 26, 2018

If I remember correctly, I believe that I had allowed finish_hook to be called with None, to notify the external Span manager that the span of the current request/current context has finished, even though python-django does not know anything about how the external entity handles/tracks the Span. This allows to completely externalize the span management, but still provide notifications about the start and finish events of the current request's span.

Taking this away, how do you suggest to track the finish event of the request's span, in case it is not tracked by this utility?

This comment has been minimized.

@carlosalberto

carlosalberto May 10, 2018

Collaborator

Hey. To me it sounds you want to do the active Span management yourself, outside the middleware, which sounds... complicated. Observe we will be issuing soon the OT 2.0 API (which includes active Span management), which may even remove the need for _current_span.

Would that solve your problem? (Observe I'd like to keep the hooks, but without the extra Span handling/not handling, if possible).

This comment has been minimized.

@philipgian

philipgian Jun 13, 2018

Yes that was the purpose. OT 2.0 API looks promising, but I haven't really followed this.

To be honest, I'm a bit out of context currently, and I wouldn't like to keep this from moving forward. We can always come back to this later.

Should we change it however, we should also change the logic around it.
Besides not calling finish_hook with None as value, we should also change the start_hook functionality. I don't see any point where we should not keep track of the span, if there is now way to notify whoever is in charge of the span that we are done.
We could simply discard the return value of the start_hook and document it accordingly.

This way, the comment by @bhs on tracer.py is also addressed

This comment has been minimized.

@bhs

bhs Jun 16, 2018

Contributor

@carlosalberto what do you think?

I would argue pretty strongly against doing active Span management in this PR – that is what the 2.0 API is for, and it will lead to extra code, bugs, or both. :-/

This comment has been minimized.

@carlosalberto

carlosalberto Jun 20, 2018

Collaborator

@bhs Oh, definitely, lets not mix them.

@philipgian Sorry for the late answer - so, yes, I think we could update this, removing the Span handling part, but keeping the (useful) hooks - POST OT 2.0, me/we can review it in case you still need any of the more advanced scenario, and put it back if still needed. Think that would work?

Let me know, thanks ;)

This comment has been minimized.

@philipgian
SERVER_SPAN_TAGS = {
ot_tags.SPAN_KIND: ot_tags.SPAN_KIND_RPC_SERVER
}

This comment has been minimized.

@carlosalberto

carlosalberto Apr 26, 2018

Collaborator

I'm wondering if, since we are here, should we include more standard tags ;)

This comment has been minimized.

@SEJeff

SEJeff Apr 29, 2018

Sure, anything specific @carlosalberto? I'm literally just forward porting @philipgian's PR, but can update it as necessary.

@carlosalberto

This comment has been minimized.

Collaborator

carlosalberto commented Aug 10, 2018

@SEJeff @philipgian Hey, was wondering if we could re-take this PR after OT 2.0 has been released ;)

Currently the new v1.0.0 branch has many improvements (full lint, travis updates, and OT 2.0 api update), so the idea would be to release an artifact from that branch in the upcoming weeks.

It would be nice to re-take this PR, mostly in the hooks part (which we don't have in the mentioned branch), if you guys have iterations ;) Else, we can have a release later to include these hooks :)

(Most likely a new PR would be needed ;) )

@SEJeff

This comment has been minimized.

SEJeff commented Aug 10, 2018

@carlosalberto Yeah totally. I'm happy to try to pick this up, or reimplement the old code (with credit to the original author) ontop of the new OT 2.x from scratch. The hooks feature seems useful, but also using the scopemanager bits also seems a bit nicer.

@SEJeff SEJeff closed this Aug 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment