Skip to content

Include parent span in Jaeger gRPC export#1809

Merged
codeboten merged 4 commits intoopen-telemetry:mainfrom
plajjan:1808-fix-jaeger-grpc-child-of
May 10, 2021
Merged

Include parent span in Jaeger gRPC export#1809
codeboten merged 4 commits intoopen-telemetry:mainfrom
plajjan:1808-fix-jaeger-grpc-child-of

Conversation

@plajjan
Copy link
Copy Markdown
Contributor

@plajjan plajjan commented May 3, 2021 •
edited
Loading

Description

This extracts the parent span and adds it as a CHILD_OF reference in the
gRPC export, so that we get the expected hierarchy of spans.

Fixes #1808

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • I have manually tested this by exporting to a Jaeger collector via gRPC and inspecting the exported data to ensure it has the correct hierarchy.
  • I have updated a unit test and the expected data conforms to my expectations, test suite passes!

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project (I believe so, it's very simple Python in this case)
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated (no docs update for a bug fix)

@plajjan plajjan requested review from a team, codeboten and srikanthccv and removed request for a team May 3, 2021 20:20
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented May 3, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@plajjan plajjan force-pushed the 1808-fix-jaeger-grpc-child-of branch from 21fe6a8 to 407d055 Compare May 3, 2021 20:33
@plajjan
Copy link
Copy Markdown
Contributor Author

plajjan commented May 3, 2021

Not sure on procedure for writing changelog.

I just managed to run the test suite and found a test case. I've updated the data there to align with the new exported data. Please check!

@plajjan plajjan force-pushed the 1808-fix-jaeger-grpc-child-of branch from 407d055 to 6127ad9 Compare May 4, 2021 19:01
@srikanthccv
Copy link
Copy Markdown
Member

srikanthccv commented May 6, 2021

You are correct. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/jaeger.md#parent-id

Jaeger Proto format does not support parent ID field; instead the parent MUST be recorded as a SpanReference of type CHILD_OF, e.g.:

@plajjan
Include parent span in Jaeger gRPC export
1111e16 
This extracts the parent span and adds it as a CHILD_OF reference in the
gRPC export, so that we get the expected hierarchy of spans.

Test case is updated to cover this case.
@plajjan plajjan force-pushed the 1808-fix-jaeger-grpc-child-of branch from 6127ad9 to 1111e16 Compare May 7, 2021 04:29
@plajjan
Copy link
Copy Markdown
Contributor Author

plajjan commented May 7, 2021

I see CI complains about a lint. I'm unable to run the lint suite locally (or I don't understand how - trying to follow instructions in CONTRIBUTING.md but won't work for me). However, I have guesstimated the fix based on the CI output and applied a fix that I have force pushed.

I have also added a changelog entry (well, I did before but never commented about it).

@codeboten @lonewolf3739 guessing one of you pressed the button to run CI. Plxz to press again :)

@plajjan
Copy link
Copy Markdown
Contributor Author

plajjan commented May 10, 2021 •
edited
Loading

CI seems to fail on something unrelated after merge of the main branch to this branch. Can it be rerun / retried?

Can I do anything?

@codeboten codeboten mentioned this pull request May 10, 2021
Closed
6 tasks
codeboten
codeboten approved these changes May 10, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @plajjan! I've added a separate PR to add the same logic to the thrift exporter #1836

@codeboten codeboten merged commit cc18b73 into open-telemetry:main May 10, 2021
@plajjan
Copy link
Copy Markdown
Contributor Author

plajjan commented May 10, 2021

Yay, thx for merging. You might want to do a follow up PR to fix the changelog. There are now two Changed sections, or maybe this is fixed during release!?

@plajjan plajjan deleted the 1808-fix-jaeger-grpc-child-of branch May 10, 2021 21:37
@codeboten
Copy link
Copy Markdown
Contributor

codeboten commented May 10, 2021

@plajjan no worries, i'll fix it in the other PR i have opened.

owais pushed a commit to owais/opentelemetry-python that referenced this pull request May 11, 2021
@plajjan @owais
Include parent span in Jaeger gRPC export (open-telemetry#1809)
f302a62 
This extracts the parent span and adds it as a CHILD_OF reference in the
gRPC export, so that we get the expected hierarchy of spans.

Test case is updated to cover this case.
This was referenced May 22, 2021
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@codeboten codeboten codeboten approved these changes

@srikanthccv srikanthccv srikanthccv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Jaeger gRPC exporter lacking span parent structure

3 participants

@plajjan @srikanthccv @codeboten
⚡