adding support for Jaeger propagator #1219

Rashmi-K-A · 2020-10-08T05:08:45Z

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #1103

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Unit tests

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added

linux-foundation-easycla · 2020-10-08T05:08:48Z

The committers are authorized under a signed CLA.

✅ Rashmi K A (ca4d6b9)

codeboten · 2020-11-03T16:23:22Z

@Rashmi-K-A are you still interested in taking on this work? #1084 has been resolved which should unblock you.

Rashmi-K-A · 2020-11-03T16:29:00Z

Hey, yes @codeboten, I noticed just today that it has been resolved. I'll get working and maybe have a PR ready by this weekend!

…etry-python

Rashmi-K-A · 2020-11-09T13:38:39Z

@codeboten The Jaeger format talks about a 'firehose' flag here. Should I be searching for a tag with a key name called firehose? I couldn't find enough information about it to implement that part.

codeboten · 2020-11-10T06:19:47Z

@codeboten The Jaeger format talks about a 'firehose' flag here. Should I be searching for a tag with a key name called firehose? I couldn't find enough information about it to implement that part.

It's a good question, i don't see any mentions of this anywhere in the js implementation. Looking through spec issues i did find this one: open-telemetry/opentelemetry-specification#231

But it looks like it's targeted for after GA, so it's probably ok to skip the implementation here.

Rashmi-K-A · 2020-11-10T06:22:01Z

@codeboten The Jaeger format talks about a 'firehose' flag here. Should I be searching for a tag with a key name called firehose? I couldn't find enough information about it to implement that part.

It's a good question, i don't see any mentions of this anywhere in the js implementation. Looking through spec issues i did find this one: open-telemetry/opentelemetry-specification#231

But it looks like it's targeted for after GA, so it's probably ok to skip the implementation here.

Cool, then I'll just add some final touches here and mark this PR as ready for review

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_format.py

carlosalberto · 2020-11-10T18:53:04Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_format.py

+            for key in getter.keys(carrier)
+            if key.startswith(self.BAGGAGE_PREFIX)
+        ]
+        for key in baggage_keys:


Worth adding support for the BAGGAGE_HEADER alternative (as a follow-up). See https://github.com/jaegertracing/jaeger-client-python/blob/master/jaeger_client/codecs.py#L116

lzchen · 2020-11-12T16:12:37Z

@Rashmi-K-A
Can you change the description of the PR to remove the "Work in progress"?

toumorokoshi

Thanks! pretty good, I think there's a few lingering questions, maybe a couple blockers here (e.g. invalid parsing of flags)

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_format.py

opentelemetry-sdk/tests/trace/propagation/test_jaeger_format.py

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_format.py

opentelemetry-sdk/tests/trace/propagation/test_jaeger_format.py

toumorokoshi · 2020-11-13T04:33:30Z

Also a changelog entry please!

ocelotl

Looking good! Just minor comments.

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_format.py

lzchen · 2020-11-19T16:25:10Z

@Rashmi-K-A
Have @toumorokoshi 's and @ocelotl 's comments been addressed yet? This change is pretty urgent.

Rashmi-K-A · 2020-11-19T16:31:05Z

hey @lzchen, I haven't gotten around to completing all of these changes yet, I'll work on getting them done by this weekend!

toumorokoshi

Thanks for addressing some of the comments! I think there's some more work that could be done, but this looks to provide most of the desired functionality. Thanks!

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_propagator.py

Rashmi-K-A · 2020-11-25T09:53:36Z

I have made one final round of changes. The B3 filename and the classname must be changed, and the extract function it uses must be imported from init.py to prevent redundancy. I'll raise an issue for that once this PR is done.

codeboten · 2020-11-30T16:23:25Z

@ocelotl & @carlosalberto please review and approve if your comments have been addressed!

ocelotl · 2020-12-03T00:27:34Z

My changes have been addressed, but the test cases are failing, @Rashmi-K-A please take a look.

ocelotl

Test cases are failing, please fix that @Rashmi-K-A 👍

codeboten · 2020-12-03T16:22:20Z

@ocelotl if you're ok with the changes, please approve. we will block on merging until the tests pass!

Rashmi-K-A · 2020-12-03T16:33:31Z

The test cases started failing after merging changes from master (which is weird because my code is in a file that's not even in master), I ll take a look within a couple of days.

ocelotl

I am ok with the changes, approving tests can be fixed before merging

…-python

Rashmi-K-A · 2020-12-06T11:43:36Z

@codeboten I fixed the errors, it turns out the propagator wasn't implementing a function added to its parent class in a recent change. It's working now! Let me know if I missed out something

adding support for Jaeger propagator

2fc6510

Rashmi-K-A force-pushed the master branch from ca4d6b9 to 2fc6510 Compare October 8, 2020 05:16

codeboten added the release:required-for-ga To be resolved before GA release label Oct 8, 2020

codeboten added this to To Do in GA Burndown Oct 16, 2020

codeboten moved this from To Do to In Progress in GA Burndown Oct 16, 2020

codeboten added the rc1 label Oct 29, 2020

codeboten added this to In progress in RC1 Oct 29, 2020

rashmi_ka added 2 commits November 6, 2020 20:17

Merge branch 'master' of github.com-personal:open-telemetry/opentelem…

dd0e90b

…etry-python

adding extractor and fixing inject method

c1eae69

Rashmi-K-A marked this pull request as ready for review November 10, 2020 15:28

Rashmi-K-A requested a review from a team as a code owner November 10, 2020 15:28

Rashmi-K-A requested review from toumorokoshi and hectorhdzg and removed request for a team November 10, 2020 15:28

carlosalberto reviewed Nov 10, 2020

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_format.py Outdated Show resolved Hide resolved

carlosalberto reviewed Nov 10, 2020

View reviewed changes

toumorokoshi suggested changes Nov 13, 2020

View reviewed changes

toumorokoshi mentioned this pull request Nov 13, 2020

Support for Jaeger's "firehose" flag. open-telemetry/opentelemetry-specification#1226

Open

ocelotl requested changes Nov 13, 2020

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_format.py Outdated Show resolved Hide resolved

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_format.py Outdated Show resolved Hide resolved

Merge branch 'master' into master

5f74efd

rashmi_ka added 2 commits November 22, 2020 21:22

Merge remote-tracking branch 'upstream/master'

fc7b964

Updated CHANGELOG

a7286ab

Rashmi-K-A requested review from carlosalberto, ocelotl and toumorokoshi November 22, 2020 16:04

toumorokoshi approved these changes Nov 24, 2020

View reviewed changes

opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/jaeger_propagator.py Outdated Show resolved Hide resolved

Adding tests and moving common extract function

021bba1

fix linting error

c8681f7

alrex added 2 commits November 30, 2020 08:23

Merge branch 'master' into master

14fcc86

Merge branch 'master' into master

e7665c8

ocelotl requested changes Dec 3, 2020

View reviewed changes

ocelotl approved these changes Dec 3, 2020

View reviewed changes

GA Burndown automation moved this from In Progress to Reviewer Approved Dec 3, 2020

ocelotl approved these changes Dec 3, 2020

View reviewed changes

lzchen and others added 5 commits December 3, 2020 16:12

Merge branch 'master' into master

fa20dd8

Merging upstream

9a375bf

Adding fields method in Jaeger propagator

8bc39ba

Merge branch 'master' of github.com-personal:Rashmi-K-A/opentelemetry…

ee9d1a9

…-python

fixing linting error

2da920a

lzchen added 2 commits December 7, 2020 12:55

Merge branch 'master' into master

553b776

Merge branch 'master' into master

13767f7

lzchen merged commit 5eb0533 into open-telemetry:master Dec 8, 2020

GA Burndown automation moved this from Reviewer Approved to Done Dec 8, 2020

RC1 automation moved this from In progress to Done Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding support for Jaeger propagator #1219

adding support for Jaeger propagator #1219

Rashmi-K-A commented Oct 8, 2020 •

edited

Loading

linux-foundation-easycla bot commented Oct 8, 2020 •

edited

Loading

codeboten commented Nov 3, 2020

Rashmi-K-A commented Nov 3, 2020

Rashmi-K-A commented Nov 9, 2020 •

edited

Loading

codeboten commented Nov 10, 2020

Rashmi-K-A commented Nov 10, 2020

carlosalberto Nov 10, 2020

lzchen commented Nov 12, 2020

toumorokoshi left a comment

toumorokoshi commented Nov 13, 2020

ocelotl left a comment

lzchen commented Nov 19, 2020 •

edited

Loading

Rashmi-K-A commented Nov 19, 2020 •

edited

Loading

toumorokoshi left a comment

Rashmi-K-A commented Nov 25, 2020 •

edited

Loading

codeboten commented Nov 30, 2020

ocelotl commented Dec 3, 2020

ocelotl left a comment

codeboten commented Dec 3, 2020

Rashmi-K-A commented Dec 3, 2020

ocelotl left a comment

Rashmi-K-A commented Dec 6, 2020

adding support for Jaeger propagator #1219

adding support for Jaeger propagator #1219

Conversation

Rashmi-K-A commented Oct 8, 2020 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

linux-foundation-easycla bot commented Oct 8, 2020 • edited Loading

codeboten commented Nov 3, 2020

Rashmi-K-A commented Nov 3, 2020

Rashmi-K-A commented Nov 9, 2020 • edited Loading

codeboten commented Nov 10, 2020

Rashmi-K-A commented Nov 10, 2020

carlosalberto Nov 10, 2020

Choose a reason for hiding this comment

lzchen commented Nov 12, 2020

toumorokoshi left a comment

Choose a reason for hiding this comment

toumorokoshi commented Nov 13, 2020

ocelotl left a comment

Choose a reason for hiding this comment

lzchen commented Nov 19, 2020 • edited Loading

Rashmi-K-A commented Nov 19, 2020 • edited Loading

toumorokoshi left a comment

Choose a reason for hiding this comment

Rashmi-K-A commented Nov 25, 2020 • edited Loading

codeboten commented Nov 30, 2020

ocelotl commented Dec 3, 2020

ocelotl left a comment

Choose a reason for hiding this comment

codeboten commented Dec 3, 2020

Rashmi-K-A commented Dec 3, 2020

ocelotl left a comment

Choose a reason for hiding this comment

Rashmi-K-A commented Dec 6, 2020

Rashmi-K-A commented Oct 8, 2020 •

edited

Loading

linux-foundation-easycla bot commented Oct 8, 2020 •

edited

Loading

Rashmi-K-A commented Nov 9, 2020 •

edited

Loading

lzchen commented Nov 19, 2020 •

edited

Loading

Rashmi-K-A commented Nov 19, 2020 •

edited

Loading

Rashmi-K-A commented Nov 25, 2020 •

edited

Loading