Django updates #6
Conversation
Use __call__ to init tracer Deal with django versions properly Add get_current_span global and deal with drf Update middleware
|
I have another tweak to make, right now it's essentially hard coded to use Jaeger, I'll fix that. |
|
Thank you for sending this out! I wanted to let you know that...
I sent this PR to a few folks who might be able to help in the meantime, though. Thank you again. |
| @@ -1,12 +1,26 @@ | |||
| from django.conf import settings | |||
| import opentracing | |||
|
|
|||
| django_tracer = None | |||
|
|
|||
| def get_tracer(): | |||
There was a problem hiding this comment.
I don't think there's need for this function, because of it always returning opentracing.tracer - moreover, I don't think we need to expose it in the public api. Or is there any use scenario where you need it?
| request = _request | ||
| return django_tracer.get_span(request) | ||
|
|
||
|
|
There was a problem hiding this comment.
So far we had been saving the span for each request in a dictionary, as part of the DjangoTracer. Is there any need to change this behaviour?
|
|
||
| def get_current_span(request): | ||
| # this lets django rest framework work seamlessly since they wrap the request | ||
| if hasattr(request, '_request'): |
There was a problem hiding this comment.
This is something we could add, yes.
| class DjangoTracer(object): | ||
| ''' | ||
| @param tracer the OpenTracing tracer to be used | ||
| to trace requests using this DjangoTracer | ||
| ''' | ||
| def __init__(self, tracer): | ||
| global django_tracer |
There was a problem hiding this comment.
Personally I don't like the idea of having a global here. We have it for other connectors, but for frameworks we have this toplevel-tracer approach, which is most likely easier/better. As before, if there's any need for this change, we can take it into consideration.
| @@ -57,7 +71,7 @@ def _apply_tracing(self, request, view_func, attributes): | |||
| k = k.lower().replace('_','-') | |||
| if k.startswith('http-'): | |||
| k = k[5:] | |||
| headers[k] = v | |||
| headers[k] = v | |||
There was a problem hiding this comment.
Usually spaces/styling changes shouldn't exist (unless we are trying to follow flake8 or similar, and then they should exist in a standalone patch).
There was a problem hiding this comment.
So my main question is: since the MiddlewareMixin class already makes __call__ work on top of process_request and process_response, what's is the need to manually expose it?
Setting the get_response field in the middleware is something we should keep, though.
tanner-bruce
force-pushed
the
master
branch
from
be25fb3 to
d7ff270
Compare
tanner-bruce
force-pushed
the
master
branch
from
d7ff270 to
30a9fc4
Compare
|
(ping @tanner-bruce) |
|
@bhs it doesn't look like @tanner-bruce is working at the same place he was when he wrote this. I've tried contacting him via a few different emails and am getting no love @carlosalberto what do you think needs to happen to get this updated and merged? Whitespace cleanup PRs (by themselves) always seem worthless, but if you're working on the file already, and it is a separate commit, it seems sensible. I'm willing to give this a go at taking on this PR (because I want to use this lib with jaeger and have little experience with either) if I could get some pointers what you'd prefer. |
|
@carlosalberto ping? @SEJeff if you are willing to take this and run with it, I think that's great. (Sorry for the insane delay, I'm cleaning my inbox.) |
|
Sorry for the long delay as well. So my take would be on removing the global tracer instance, removing the Hope that helps. Let us know @SEJeff ;) |
|
@carlosalberto Sure, when my personal laptop (which decided to totally corrupt the drive after a raid controller fw update 😢) is revived, I'll give it another go! It would still be easier to test things after you release opentracing 2.0 hint hint |
|
@SEJeff Feel free to test this case - otherwise, I've added a profile to Travis so we can test against versions using the old-style Middleware (2.9), and also a newer one (2.11). And both seem to be working just fine https://travis-ci.org/opentracing-contrib/python-django/builds/414348874 Let me know ;) |
@bhs I hope you are the right person to ping! This deals with Django >= 1.10 as well as Django REST Framework
Use call to init tracer
Deal with django versions properl
Add get_current_span global and deal with drf
Update middleware