Export span status to Jaeger #367

dgzlopes · 2020-01-16T13:16:03Z

Sets status tags on the Jaeger span.
Removes TODO from the Jaeger integration that points to a closed issue with a merged PR.

Signed-off-by: Daniel González Lopes [email protected]

Signed-off-by: Daniel González Lopes <[email protected]>

codecov-io · 2020-01-16T13:26:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e9a480f). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #367   +/-   ##
=========================================
  Coverage          ?   85.26%           
=========================================
  Files             ?       38           
  Lines             ?     1920           
  Branches          ?      225           
=========================================
  Hits              ?     1637           
  Misses            ?      219           
  Partials          ?       64

Impacted Files	Coverage Δ
...-ext-flask/src/opentelemetry/ext/flask/__init__.py	`88.88% <100%> (ø)`
opentelemetry-api/src/opentelemetry/util/types.py	`100% <100%> (ø)`
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py	`86.54% <100%> (ø)`
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`90.94% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9a480f...ae76533. Read the comment docs.

mauriciovasquezbernal · 2020-01-16T19:00:09Z

I'm wondering if we should try to export the status in some way to Jaeger. Looking at https://github.com/jaegertracing/jaeger-idl/blob/master/thrift/jaeger.thrift it appears to me that there is not any status related field. Perhaps it could be encoded as a tag?

dgzlopes · 2020-01-16T19:27:54Z

It sounds like a good idea @mauriciovasquezbernal.

From taking a quick look at the Go client, I think that they are doing exactly that.

mauriciovasquezbernal · 2020-01-17T02:23:37Z

@dgzlopes do you have plans to implement that? If not I'll open an issue to keep track of it.

dgzlopes · 2020-01-17T16:26:58Z

Sure, I'm happy to implement it @mauriciovasquezbernal. Meanwhile, I'm going to change the title and the status of this pull request.

dgzlopes · 2020-01-17T16:41:28Z

Also, from taking a closer look at the Go implementation I see that they set an "error" tag in the Jaeger span if Status.Code is not OK. Do you think we should add that tag too?

codeboten · 2020-01-17T17:44:34Z

Yes! That would be awesome.

Oberon00 · 2020-01-20T10:14:37Z

I think this is something that should be defined in the spec, similar to open-telemetry/opentelemetry-specification#380

mauriciovasquezbernal · 2020-01-20T16:12:01Z

I opened an issue in the specs repo to track it open-telemetry/opentelemetry-specification#414.

Signed-off-by: Daniel González Lopes <[email protected]>

dgzlopes · 2020-01-22T11:18:20Z

I'm researching why Travis is failing. Locally all the test-ext-jaeger tests are passing.

Update: Now I'm able to reproduce it locally :)

Based on the changes of open-telemetry#358 Signed-off-by: Daniel González Lopes <[email protected]>

Signed-off-by: Daniel González Lopes <[email protected]>

mauriciovasquezbernal

It looks good to me, just two small nits.
Thanks!

ext/opentelemetry-ext-jaeger/tests/test_jaeger_exporter.py

ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py

Signed-off-by: dgzlopes <[email protected]>

dgzlopes

Fixed nits related with magic numbers on tests :)

codeboten

Thanks so much for tackling this. Just one change requested, the rest looks good!

codeboten · 2020-01-22T16:29:48Z

ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py

-        # TODO: status is missing:
-        # https://github.com/open-telemetry/opentelemetry-python/issues/98
+        if tags is None:
+            tags = []


It might be easier to change _extract_tags to always return an empty list, rather than checking this here

And in fact we probably should since the only other place where it's used also ends up doing some juggling around this:

opentelemetry-python/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py

Lines 239 to 243 in ed8cbdc

fields = []

if event.attributes is not None:

fields = _extract_tags(event.attributes)

fields.append(

Fixed! Added the check when span.status wasn't mandatory, but now it makes no sense.

mind updating the in _extract_logs_from_span as well?

But if we change _extract_logs_from_span to return an empty list when a span has no logs tests break because they expect logs to be None. I think that it's the same with refs.

# Example output form tests Span([348 chars]Double=None, vBool=None, vLong=None, vBinary=None)], logs=[]) Span([348 chars]Double=None, vBool=None, vLong=None, vBinary=None)], logs=None)

Maybe I'm misunderstanding something. What do you think?

Sorry, I meant to update the code in _extract_logs_from_span to take advantage of this new empty list from _extract_tags, lines 239 and 240 are no longer needed:

opentelemetry-python/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py

Lines 239 to 243 in ed8cbdc

fields = []

if event.attributes is not None:

fields = _extract_tags(event.attributes)

fields.append(

Oh, okay! It should be updated now.

Signed-off-by: dgzlopes <[email protected]>

Signed-off-by: Daniel González Lopes <[email protected]>

codeboten

Looks good to me.

mauriciovasquezbernal

Thanks for the PR, it looks good to me.
Just pushed a commit to update the changelog for it.

Remove status is missing TODO

b39f33b

Signed-off-by: Daniel González Lopes <[email protected]>

dgzlopes requested a review from a team January 16, 2020 13:16

dgzlopes changed the title ~~Remove status is missing TODO~~ [WIP] Export span status to Jaeger Jan 17, 2020

mauriciovasquezbernal mentioned this pull request Jan 20, 2020

Define OpenTelemetry to Jaeger span Transformation open-telemetry/opentelemetry-specification#414

Closed

Export status to Jaeger

e9a480f

Signed-off-by: Daniel González Lopes <[email protected]>

dgzlopes changed the title ~~[WIP] Export span status to Jaeger~~ Export span status to Jaeger Jan 22, 2020

dgzlopes added 3 commits January 22, 2020 13:05

Merge branch 'master' into remove-status-is-missing-todo

5232a8b

Fix Jaeger status

583279c

Based on the changes of open-telemetry#358 Signed-off-by: Daniel González Lopes <[email protected]>

Fixing linting error

1c8f198

Signed-off-by: Daniel González Lopes <[email protected]>

mauriciovasquezbernal reviewed Jan 22, 2020

View reviewed changes

ext/opentelemetry-ext-jaeger/tests/test_jaeger_exporter.py Outdated Show resolved Hide resolved

ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py Show resolved Hide resolved

Fix nits

ed8cbdc

Signed-off-by: dgzlopes <[email protected]>

dgzlopes commented Jan 22, 2020

View reviewed changes

codeboten suggested changes Jan 22, 2020

View reviewed changes

dgzlopes added 2 commits January 22, 2020 17:52

Change _extract_tags to return an empty list

0a15900

Signed-off-by: dgzlopes <[email protected]>

Remove None check

30bc36a

Signed-off-by: Daniel González Lopes <[email protected]>

codeboten approved these changes Jan 22, 2020

View reviewed changes

ext/jaeger: update CHANGELOG

ae76533

mauriciovasquezbernal approved these changes Jan 23, 2020

View reviewed changes

mauriciovasquezbernal mentioned this pull request Jan 28, 2020

Jeager exporter fails on events with empty attributes #383

Closed

mauriciovasquezbernal added the exporters label Jan 28, 2020

mauriciovasquezbernal added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jan 28, 2020

mauriciovasquezbernal mentioned this pull request Jan 28, 2020

ext/jaeger: add span kind jaeger exporter #387

Merged

c24t approved these changes Jan 29, 2020

View reviewed changes

c24t merged commit 3995075 into open-telemetry:master Jan 29, 2020

c24t removed the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jan 29, 2020

toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Feb 17, 2020

Export span status to Jaeger (open-telemetry#367)

6bf2e4f

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

docs(node-sdk): extend README (open-telemetry#367)

fe53994

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export span status to Jaeger #367

Export span status to Jaeger #367

dgzlopes commented Jan 16, 2020 •

edited

Loading

codecov-io commented Jan 16, 2020 •

edited

Loading

mauriciovasquezbernal commented Jan 16, 2020

dgzlopes commented Jan 16, 2020

mauriciovasquezbernal commented Jan 17, 2020

dgzlopes commented Jan 17, 2020

dgzlopes commented Jan 17, 2020

codeboten commented Jan 17, 2020

Oberon00 commented Jan 20, 2020

mauriciovasquezbernal commented Jan 20, 2020

dgzlopes commented Jan 22, 2020 •

edited

Loading

mauriciovasquezbernal left a comment

dgzlopes left a comment

codeboten left a comment

codeboten Jan 22, 2020

codeboten Jan 22, 2020

dgzlopes Jan 22, 2020 •

edited

Loading

codeboten Jan 22, 2020

dgzlopes Jan 22, 2020

codeboten Jan 22, 2020

dgzlopes Jan 22, 2020 •

edited

Loading

codeboten left a comment

mauriciovasquezbernal left a comment

	fields = []
	if event.attributes is not None:
	fields = _extract_tags(event.attributes)

	fields.append(

Export span status to Jaeger #367

Export span status to Jaeger #367

Conversation

dgzlopes commented Jan 16, 2020 • edited Loading

codecov-io commented Jan 16, 2020 • edited Loading

Codecov Report

mauriciovasquezbernal commented Jan 16, 2020

dgzlopes commented Jan 16, 2020

mauriciovasquezbernal commented Jan 17, 2020

dgzlopes commented Jan 17, 2020

dgzlopes commented Jan 17, 2020

codeboten commented Jan 17, 2020

Oberon00 commented Jan 20, 2020

mauriciovasquezbernal commented Jan 20, 2020

dgzlopes commented Jan 22, 2020 • edited Loading

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

dgzlopes left a comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

codeboten Jan 22, 2020

Choose a reason for hiding this comment

codeboten Jan 22, 2020

Choose a reason for hiding this comment

dgzlopes Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

codeboten Jan 22, 2020

Choose a reason for hiding this comment

dgzlopes Jan 22, 2020

Choose a reason for hiding this comment

codeboten Jan 22, 2020

Choose a reason for hiding this comment

dgzlopes Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

dgzlopes commented Jan 16, 2020 •

edited

Loading

codecov-io commented Jan 16, 2020 •

edited

Loading

dgzlopes commented Jan 22, 2020 •

edited

Loading

dgzlopes Jan 22, 2020 •

edited

Loading

dgzlopes Jan 22, 2020 •

edited

Loading