Opentracing bridge #388
Opentracing bridge #388
Conversation
| In other words, | ||
| it translates the calls to the OpenTracing API to Elastic APM and thus allows for reusing existing instrumentation. | ||
|
|
||
| The first span of a service will be converted to an Elastic APM |
There was a problem hiding this comment.
first span of a service trace / operation?
There was a problem hiding this comment.
Saying "trace" here might be misleading, as a trace represents the request through multiple services.
But I agree that it is not super clear what is meant here. I'd love if opentracing would define something like an entry span, but they do not.
| appears in the "User" tab in the transaction details in the Elastic APM UI | ||
| - `result` - sets the result of the transaction. Overrides the default value of `success`. | ||
|
|
||
| NOTE: these tags need to be set on the first span of an operation (e.g. an incoming request of your webapp). |
There was a problem hiding this comment.
users still can set any arbitrary tags on spans, right? only that they will not be mapped to know fields. Ill rephrase L61 to something like: "Some tags are well defined in Elastic APM. If you defined these tags in the first span of an operation, the will be automatically mapped to defined fields..." and remove this Note
docs/opentracing.asciidoc
Outdated
| [[opentracing-instrumentation]] | ||
| ==== Instrumentation | ||
|
|
||
| It is recommended to not use the built-in instrumentations of Elastic APM together with third-party OpenTracing instrumentations like https://pypi.org/project/opentracing_instrumentation/[opentracing_instrumentation]. |
There was a problem hiding this comment.
I'll clarify that the advise is to not use 2 different instrumentation libraries for the same service, as the whole point of opentracing is to be able to mix different vendors for DT.
There was a problem hiding this comment.
(side note: OpenTracing and distributed tracing aren't really related, and I don't think a goal of opentracing is to mix vendors. As far as I understand, the main goal of opentracing is to easily allow the user to switch vendor without (too many) code changes, but it doesn't define a common trace propagation protocol like the W3C traceparent (hopefully-soon-)standard does, and it says nothing about storage or data model.)
Do I understand you correctly that you'd like to add "in the same service" to the above sentence? That seems reasonable.
elasticapm/context/threadlocal.py
Outdated
| thread_local = threading.local() | ||
| thread_local.transaction = None | ||
| from elasticapm.context.base import BaseContext | ||
|
|
||
| elasticapm_span_var = None |
There was a problem hiding this comment.
because I forgot to remove it when refactoring this whole bit :) good catch!
| def __init__(self, client_instance=None, config=None, scope_manager=None): | ||
| self._agent = client_instance or self._elasticapm_client_class(config=config) | ||
| if scope_manager and not isinstance(scope_manager, ThreadLocalScopeManager): | ||
| warnings.warn( |
There was a problem hiding this comment.
if not supported, instead of issuing a use-at-your-own-peril warning isn't just better to ignore it and set self._scope_manager to ThreadLocalScopeManager()?
There was a problem hiding this comment.
Well, things will fail in hilarious ways anyway if you use asyncio or gevent with a ThreadLocalScopeManager. With this, people have at least the choice what kind of hilarity they want. Also, I hope to get at least asyncio covered Real Soon Now ™️ (maybe even this decade!)
| else None | ||
| ) | ||
| span = transaction.begin_span(operation_name, None, parent_span_id=parent_span_id) | ||
| trace_parent = parent_context.trace_parent if parent_context else transaction.trace_parent |
There was a problem hiding this comment.
just to wrap my head around this, parent_context.trace_parent is not None when the span's parent is propagated from another service, right? if so, parent_context.trace_parent is not None means that transaction is None (because this is the first span of a service)?
another way of putting it: i actually don't understand this bit 🙈
| ot_span.set_tag(k, v) | ||
| return ot_span | ||
|
|
||
| def extract(self, format, carrier): |
There was a problem hiding this comment.
why some errors return an exception (like carrier not supported) and others do not (like elastic-apm-traceparent header bad formatted)?
| [[opentracing-baggage]] | ||
| ==== Baggage | ||
| The `Span.set_baggage_item(key, value)` method is not supported. | ||
| Baggage items are silently dropped. |
There was a problem hiding this comment.
how this exactly happens? i was expecting to have set_baggage_item method implemented as a noop, but i don't find it
There was a problem hiding this comment.
We inherit the noop-method from the super class
There was a problem hiding this comment.
could use a test, for some more explicitness 🙈
There was a problem hiding this comment.
done in 4ca8859 (plus another few tests plus a fix for an issue I found thanks to those tests. "write more tests" is the lesson here I guess)
| return self | ||
|
|
||
| def set_tag(self, key, value): | ||
| if self.is_transaction: |
There was a problem hiding this comment.
aren't a few known tags missing? context.request.cookies, context.request.headers, context.response...?
elasticapm/traces.py
Outdated
| @@ -48,8 +48,8 @@ def __init__(self, tracer, transaction_type="custom", trace_parent=None, is_samp | |||
| def end_transaction(self): | |||
| self.duration = _time_func() - self.start_time | |||
|
|
|||
| def begin_span(self, name, span_type, context=None, leaf=False, tags=None): | |||
| parent_span = get_span() | |||
| def begin_span(self, name, span_type, context=None, leaf=False, tags=None, parent_span_id=None): | |||
There was a problem hiding this comment.
is this part of the agent API? if so, what do you think about making this private and keep a public begin_span without the parent_span_id argument?
There was a problem hiding this comment.
Did you think of something like this? d33852d
No description provided.