ext/jaeger - Transform resource to tags when exporting #645
Conversation
| @@ -197,6 +197,8 @@ def _translate_to_jaeger(spans: Span): | |||
| parent_id = span.parent.span_id if span.parent else 0 | |||
|
|
|||
| tags = _extract_tags(span.attributes) | |||
| if span.resource is not None: | |||
There was a problem hiding this comment.
I'm wondering how we can make this test more efficient. It looks to me that resource is never None but it is _EMPTY_RESOURCE.
I think one solution could be to expose _EMPTY_RESOURCE (remove the _) and make a comparison using the is operator.
if span.resource is not ̣̣̣resources.EMPTY_RESOURCE:
There was a problem hiding this comment.
I made the suggested change.
Though notice, I had to create an empty resource in all of the spans used for the jaeger tests, as it is not created by default when accessing the trace.Span directly.
I guess that in the real flow it will always be created, but maybe it's something to consider and we should keep the is not None check nonetheless, just to play it safe.
What do you think?
There was a problem hiding this comment.
Good point. TBH I'm not 100% what's the best approach here, I made that suggestion to make it more efficient, but probably it's not worthy as this check is very fast in both cases and we haven't done an optimization in other places yet. I'm fine with both solutions, let's see other people's opinions on this.
|
Btw, there are some style fails. You can check those locally by running "tox -e lint". |
|
Hey, @toumorokoshi could you please review this PR? 🙏🏻 |
| @@ -197,6 +198,8 @@ def _translate_to_jaeger(spans: Span): | |||
| parent_id = span.parent.span_id if span.parent else 0 | |||
|
|
|||
| tags = _extract_tags(span.attributes) | |||
| if span.resource is not EMPTY_RESOURCE: | |||
There was a problem hiding this comment.
is this necessary? feels like it adds conditional logic for a very nominal (maybe 500ns on a modern processor at most) improvement.
There was a problem hiding this comment.
also I don't believe this is completely correct. Currently in the SDK, the Span.resource attribute can be None as well.
This can be simplified to a Falsy check (if span.resource) to cover both cases.
There was a problem hiding this comment.
Ok, so I'm gonna revert the exposing of EMPTY_RESOURCE and just make the simple Falsy check.
@mauriciovasquezbernal, are you ok with that?
There was a problem hiding this comment.
Yes, I'm ok with that, sorry for the confusion.
| @@ -34,7 +35,9 @@ def setUp(self): | |||
| is_remote=False, | |||
| ) | |||
|
|
|||
| self._test_span = trace.Span("test_span", context=context) | |||
| self._test_span = trace.Span( | |||
There was a problem hiding this comment.
if you always attach an empty resource by default, you're omitting the valid test case of a Span with "None" as the resource.
For the purposes of this PR, I recommend adding a case that assumes the Span.resource may be None.
Added #702 for a more global fix.
There was a problem hiding this comment.
Thanks for reviewing @toumorokoshi, I made the requested changes.
Do you think I should add another test case explicitly handling the None case?
Or is it good enough that it's contained in the other tests?
This PR implements the missing part of exporting the TraceProvider resource into Jaeger.
Same as in js.