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

Initialization of the global tracer #23

Closed
carlosalberto opened this issue May 11, 2018 · 10 comments
Closed

Initialization of the global tracer #23

carlosalberto opened this issue May 11, 2018 · 10 comments

Comments

@carlosalberto
Copy link
Collaborator

Hey @mikebryant

I was checking the initialization of the global tracer, and I'm not sure this is the right approach. Specifically this is the part I'm talking about:

        if hasattr(settings, 'OPENTRACING_TRACER'):
            # Backwards compatibility with the old way of defining the tracer
            opentracing.tracer = settings.OPENTRACING_TRACER._tracer

Usually, they way we have done things for other instrumentation in Java, is to do it the other way around: if the user doesn't specify explicitly a tracer (through OPENTRACING_TRACER or OPENTRACING_TRACER_CALLABLE in this case), then fallback to opentracing.tracer).

Setting the opentracing.tracer instance right from python-django kinda breaks the contract, and I'm afraid that the user won't be able to set the global tracer himself if we do this - and also, I'm afraid other instrumentation libraries may also want to set opentracing.tracer each, and we end up in an override cascade.

Is there any need from your part to have this code running? Let me know, and maybe we can find a solution in the middle ;)

@carlosalberto
Copy link
Collaborator Author

Fixed by #29

@mikebryant Let me know if you have any opinion or feedback on this ;)

@mikebryant
Copy link
Collaborator

Setting a tracer object yourself can be pretty hard, in a Django environment. You need to be able to have that happen lazily, after forking has happened (see all the issues with Jaeger)

Expecting the user to initialise the tracer themselves in the settings module means it's impossible to use - there's no way of initialising the tracer at the right time.

@mikebryant
Copy link
Collaborator

Have you got an example of being able to use Jaeger with Django under mod_wsgi using the approach you're proposing?

@carlosalberto
Copy link
Collaborator Author

You need to be able to have that happen lazily, after forking has happened (see all the issues with Jaeger)

Not really. I went through https://github.com/jaegertracing/jaeger-client-python/issues and didn't manage to find a related one...

Have you got an example of being able to use Jaeger with Django under mod_wsgi using the approach you're proposing?

So I haven't set up Django + mod_wsgi myself, but some (production) cases I have found work by putting all the parameters needed for initializing your tracer right in settings.py:

# settings.py

TRACER_KEY = "dasd7ad87a"
TRACER_SERVICE = "DjangoServer01"
TRACER = MyTracerImpl(TRACER_KEY, TRACER_SERVICE) # create specific tracer implementation
opentracing.tracer = TRACER

...

OPENTRACING_TRACER = DjangoTracer(opentracing.tracer)

One alternative I see nevertheless is providing, based on very Django's needs, a callback at DjangoTracer set-up time:

OPENTRACING_TRACER_INIT_CB = my_init_cb

# In some place:
def my_init_cb(django_tracer):
   opentracing.tracer = django_tracer._tracer

Would that work?

@carlosalberto
Copy link
Collaborator Author

@mikebryant Guess you were talking about #14 ? ;)

@mikebryant
Copy link
Collaborator

I believe the issue was jaegertracing/jaeger-client-python#60, leading to deadlocks when the tracer initialisation was done in settings.py

The problem as I recall is needing to start the event loop after forking, which can't be done in settings.py, as that's read pre-fork.

My requirements are fairly simple, I want to be able to have static Django settings that just work, and I don't really want to have to stick python code somewhere to set up the global tracer object.

  • I want to be able to use opentracing.tracer inside python libraries, so I can inject spans and so on. I don't want to have to plumb a tracer object throughout every single function call I ever make, so I need to use the global for that.
  • I want to be able to use python-django to set up my global tracer, so I can configure it in one place. I won't want to need to write my own lazy middlewhere to wait until the forking has happened, and then set up the tracer object.

@carlosalberto
Copy link
Collaborator Author

I want to be able to use python-django to set up my global tracer, so I can configure it in one place.

Sadly this could be requested in any instrumentation library (gevent, asyncio, tornado, sqlalchemy, you name it). This wouldn't scale. And in Java, C# and Javascript the duty of the developer is to take the extra steps to write code to set the global Tracer and consume it later on. And nobody had complained so far.

In any case, I think at least I'd like to have this as an optional OPENTRACING_SET_GLOBAL_TRACER value (False by default) - nobody I know has requested/asked this feature -and personally I'm not sure whether it's correct or not-, but if it is needed in some scenarios, we can definitely provide it.

I will cook re-work so initializing this one is at least optional.

@mikebryant
Copy link
Collaborator

I don't mind having it hidden behind a setting.

I can put this another way, we have many django projects deployed, and I don't want to write code repeatedly. The last time I tried setting this up, I found that I couldn't instantiate the Jaeger tracing object in settings.py without deadlocking the system. And writing my own lazy initialization middleware in every project isn't optimal, so I want to push it back upstream, so there's a simple way of making Jaeger work.

As long as it's simple and obvious to set up this library in a way that satisfies the following requirements, I'll be happy

  • The global tracer is set up correctly
  • The django internals are set up correctly

(Not saying the library must do both of those in all cases, but I don't want to need to write more than a few lines of code in every single project to achieve this

@carlosalberto
Copy link
Collaborator Author

#30 should do it. @mikebryant take a look whenever you have time, please.

@carlosalberto
Copy link
Collaborator Author

Closing as #30 handles this issue by making this bit optional.

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

2 participants