Skip to content

Setup instructions in conjunction with jaeger-client-python leads to pitfall #7

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

Closed
bitglue opened this issue Sep 12, 2017 · 8 comments

Comments

@bitglue
Copy link

bitglue commented Sep 12, 2017

See jaegertracing/jaeger-client-python#60. Doing the obvious setup, according to the readme of these two projects, leads to a deadlock which is quite tricky to sort out.

I've not yet found an elegant workaround, though I'm not an expert in Jaeger, or Django. In the linked issue, it's suggested the Jaeger tracer not be initialized until after the WSGI server (gunicorn in my case) forks. That seems like a good idea, even if it didn't deadlock.

The closest solution would seem to be putting the initialization of the Jaeger tracer in gunicorns post_fork() hook (or similar for other WSGI servers). Trouble is by that time I'd think OpenTracingMiddleware has already saved a reference to the tracer. So I'm looking at doing something ugly.

@dudymas
Copy link

dudymas commented Nov 16, 2017

So it looks like someone made a fix for this already in #6 , which is from work on the issue at jaegertracing/jaeger-client-python#69
The PR appears to not be approved because the code has jaeger_client hardcoded in and also the maintainer hasn't been given a clear enough picture as to why this solution has to be in place.

@dudymas
Copy link

dudymas commented Nov 16, 2017

I'm hoping @tanner-bruce , @bhs , or @carlosalberto could comment, as we could use this fix too.

@tanner-bruce
Copy link

@dudymas I'm hoping to get back to it really soon, simply haven't had a chance. There are a few things that need to be fixed in the PR, as well as a change or two and I need to rationalize some of the decisions. I also want to test a different approach to storing the root span that would make the middleware arguably cleaner.

@dudymas
Copy link

dudymas commented Nov 21, 2017

@tanner-bruce would you mind if I make a PR to your repo? I already removed the jaeger client dep and also dried up the middleware to use more of the django helper class for init

@tanner-bruce
Copy link

@dudymas sure, thanks for putting in the work.

@ror6ax
Copy link

ror6ax commented Dec 10, 2017

Hi guys, thanks for your work on this! Any idea when can we expect this in master? If there's any work that still needs to be done, I'd be happy to do that...

@mikebryant
Copy link
Collaborator

@ror6ax I've tried a different way of approaching this in #9

@ror6ax
Copy link

ror6ax commented Dec 20, 2017

@mikebryant - awesome! Hopefully this gets merged soon...

@bhs bhs closed this as completed in e625a63 Dec 30, 2017
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

5 participants