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.

Sign up for GitHub

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

ext/jaeger - Transform resource to tags when exporting #645

Merged
merged 9 commits into from
May 19, 2020
Merged

ext/jaeger - Transform resource to tags when exporting #645

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
91db3cd
ext/jaeger - Transform resource to tags when exporting
nirsky May 4, 2020
d756f58
Check for empty resource instead of None
nirsky May 5, 2020
1d20465
Merge branch 'master' into feat/jaeger/resource
nirsky May 5, 2020
82e103a
linting
nirsky May 5, 2020
7d4ba99
Merge branch 'feat/jaeger/resource' of https://github.com/nirsky/open…
nirsky May 5, 2020
12da24a
linting
nirsky May 5, 2020
007165e
Merge branch 'master' into feat/jaeger/resource
nirsky May 6, 2020
6559eec
Merge branch 'master' into feat/jaeger/resource
toumorokoshi May 17, 2020
3127e73
Revert exposing resource
nirsky May 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import opentelemetry.trace as trace_api
from opentelemetry.ext.jaeger.gen.agent import Agent as agent
from opentelemetry.ext.jaeger.gen.jaeger import Collector as jaeger
from opentelemetry.sdk.resources import EMPTY_RESOURCE
from opentelemetry.sdk.trace.export import Span, SpanExporter, SpanExportResult
from opentelemetry.trace.status import StatusCanonicalCode

Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary? feels like it adds conditional logic for a very nominal (maybe 500ns on a modern processor at most) improvement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I'm gonna revert the exposing of EMPTY_RESOURCE and just make the simple Falsy check.
@mauriciovasquezbernal, are you ok with that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm ok with that, sorry for the confusion.

tags.extend(_extract_tags(span.resource.labels))

tags.extend(
[
Expand Down
25 changes: 22 additions & 3 deletions ext/opentelemetry-ext-jaeger/tests/test_jaeger_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from opentelemetry import trace as trace_api
from opentelemetry.ext.jaeger.gen.jaeger import ttypes as jaeger
from opentelemetry.sdk import trace
from opentelemetry.sdk.trace import Resource
from opentelemetry.trace.status import Status, StatusCanonicalCode


Expand All @@ -34,7 +35,9 @@ def setUp(self):
is_remote=False,
)

self._test_span = trace.Span("test_span", context=context)
self._test_span = trace.Span(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"test_span", context=context, resource=Resource.create_empty()
)
self._test_span.start()
self._test_span.end()

Expand Down Expand Up @@ -189,16 +192,27 @@ def test_translate_to_jaeger(self):
kind=trace_api.SpanKind.CLIENT,
),
trace.Span(
name=span_names[1], context=parent_context, parent=None
name=span_names[1],
context=parent_context,
parent=None,
resource=Resource.create_empty(),
),
trace.Span(
name=span_names[2],
context=other_context,
parent=None,
resource=Resource.create_empty(),
),
trace.Span(name=span_names[2], context=other_context, parent=None),
]

otel_spans[0].start(start_time=start_times[0])
# added here to preserve order
otel_spans[0].set_attribute("key_bool", False)
otel_spans[0].set_attribute("key_string", "hello_world")
otel_spans[0].set_attribute("key_float", 111.22)
otel_spans[0].resource = Resource(
labels={"key_resource": "some_resource"}
)
otel_spans[0].set_status(
Status(StatusCanonicalCode.UNKNOWN, "Example description")
)
Expand Down Expand Up @@ -237,6 +251,11 @@ def test_translate_to_jaeger(self):
vType=jaeger.TagType.DOUBLE,
vDouble=111.22,
),
jaeger.Tag(
key="key_resource",
vType=jaeger.TagType.STRING,
vStr="some_resource",
),
jaeger.Tag(
key="status.code",
vType=jaeger.TagType.LONG,
Expand Down
6 changes: 3 additions & 3 deletions opentelemetry-sdk/src/opentelemetry/sdk/resources/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ def __init__(self, labels: Labels):
@staticmethod
def create(labels: Labels) -> "Resource":
if not labels:
return _EMPTY_RESOURCE
return EMPTY_RESOURCE
return Resource(labels)

@staticmethod
def create_empty() -> "Resource":
return _EMPTY_RESOURCE
return EMPTY_RESOURCE

@property
def labels(self) -> Labels:
Expand All @@ -50,4 +50,4 @@ def __eq__(self, other: object) -> bool:
return self._labels == other._labels


_EMPTY_RESOURCE = Resource({})
EMPTY_RESOURCE = Resource({})
2 changes: 1 addition & 1 deletion opentelemetry-sdk/tests/metrics/test_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def test_resource_empty(self):
meter_provider = metrics.MeterProvider()
meter = meter_provider.get_meter(__name__)
# pylint: disable=protected-access
self.assertIs(meter.resource, resources._EMPTY_RESOURCE)
self.assertIs(meter.resource, resources.EMPTY_RESOURCE)


class TestMeter(unittest.TestCase):
Expand Down
6 changes: 3 additions & 3 deletions opentelemetry-sdk/tests/resources/test_resources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@ def test_create(self):
self.assertEqual(resource.labels, labels)

resource = resources.Resource.create_empty()
self.assertIs(resource, resources._EMPTY_RESOURCE)
self.assertIs(resource, resources.EMPTY_RESOURCE)

resource = resources.Resource.create(None)
self.assertIs(resource, resources._EMPTY_RESOURCE)
self.assertIs(resource, resources.EMPTY_RESOURCE)

resource = resources.Resource.create({})
self.assertIs(resource, resources._EMPTY_RESOURCE)
self.assertIs(resource, resources.EMPTY_RESOURCE)

def test_resource_merge(self):
left = resources.Resource({"service": "ui"})
Expand Down
2 changes: 1 addition & 1 deletion opentelemetry-sdk/tests/trace/test_trace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ def test_default_span_resource(self):
tracer = tracer_provider.get_tracer(__name__)
span = tracer.start_span("root")
# pylint: disable=protected-access
self.assertIs(span.resource, resources._EMPTY_RESOURCE)
self.assertIs(span.resource, resources.EMPTY_RESOURCE)

def test_span_context_remote_flag(self):
tracer = new_tracer()
Expand Down