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

start_span on FlaskTracer should delegate to the underlying tracer implementation #17

Open
jscn opened this issue May 31, 2018 · 4 comments

Comments

@jscn
Copy link

jscn commented May 31, 2018

I'm trying to manually add a span to a trace using the jaeger Python client, as described in the documentation. However in the example, the span is created by calling the underlying tracer implementation directly (jaeger_tracer.start_span() rather than the FlaskTracer instance.

I didn't notice this initially and attempted to call start_span() on my FlaskTracer instance directly, which resulted in an error being raised from the underlying opentracing.Tracer:

Traceback (most recent call last):
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/app.py", line 2301, in __call__
    return self.wsgi_app(environ, start_response)
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/app.py", line 2287, in wsgi_app
    response = self.handle_exception(e)
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/app.py", line 1733, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/_compat.py", line 35, in reraise
    raise value
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/app.py", line 2284, in wsgi_app
    response = self.full_dispatch_request()
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/app.py", line 1807, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/app.py", line 1710, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/_compat.py", line 35, in reraise
    raise value
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/app.py", line 1805, in full_dispatch_request
    rv = self.dispatch_request()
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/flask/app.py", line 1791, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/jscn/work/ent-admin/entadmin/api/people/people.py", line 23, in read
    person = PersonRepository().me()
  File "/home/jscn/work/ent-admin/entadmin/repositories/person.py", line 17, in me
    person = self._get_one()
  File "/home/jscn/work/ent-admin/entadmin/repositories/_base.py", line 80, in _get_one
    response = self._get(url)
  File "/home/jscn/work/ent-admin/entadmin/repositories/_base.py", line 68, in _get
    with tracer.start_span('client get', child_of=parent_span) as span:
  File "/home/jscn/venvs/ent-admin/lib/python3.4/site-packages/opentracing/tracer.py", line 88, in start_span
    return self._noop_span
AttributeError: 'Tracer' object has no attribute '_noop_span'```

So it looks like, in order to make this work, I need to always pass around both the FlaskTracer instance, and the underlying jaeger_client.Tracer instance.

Since FlaskTracer already has a reference to that jaeger_client.Tracer instance, and that instance's implementation of start_span actually works, I think that FlaskTracer should delegate calls to start_span to whichever Tracer is passed in when FlaskTracer is instantiated.

@jscn
Copy link
Author

jscn commented May 31, 2018

As far as I can tell, there's no reason for FlaskTracer to inherit from opentracing.Tracer at all: FlaskTracer doesn't override any methods of the parent opentracing.Tracer class, and calling those methods on the opentracing.Tracer doesn't work because the parent class is never initialised. (Hence the above traceback). Furthermore, all the actual tracing in FlaskTracer is done via the tracer which is provided at instantiation time.

I do think it would make sense for FlaskTracer to implement all the methods of opentracing.Tracer, but delegate them to the tracer which is passed in when FlaskTracer is instantiated (in this case, jaeger_client.Tracer).

While I'm personally not a great fan of explicit interfaces in Python, if you want to encode the fact that FlaskTracer should provide the same interface as opentracing.Tracer, I'd be happy to open a ticket / PR on the the upstream project to provide an ABC which FlaskTracer could inherit from.

@carlosalberto
Copy link
Contributor

@jscn Actually the 'bad' name part was mentioned in the past - DjangoTracer and FlaskTracer implementations should be changed to DjangoTracing and FlaskTracing respectively, and maybe offer an actual public property exposing the underlying tracer (so you could do tracing.tracer.start_span() for any tracer and/or instrumentation library*).

Still, I think we should track that (and make it a priority once we land the OT 2.0 version ;) )

  • That's also without mentioning a proper global Tracer, which we plan to have -like Java or C# do- shortly after the mentioned OT 2.0 integration happens.

@IngaFeick
Copy link

@jscn I know it's been a while since you raised this but do you by chance still remember what it was exactly that you did to work around this? I'm having the same issue. Thanks!

@v-yzakapko
Copy link

v-yzakapko commented Jun 11, 2020

@jscn I know it's been a while since you raised this but do you by chance still remember what it was exactly that you did to work around this? I'm having the same issue. Thanks!

I am having this issue too

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

No branches or pull requests

4 participants