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.

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

[receiver/skywalkingreceiver] Add extra link attributes from skywalking ref. #12651

Merged
merged 2 commits into from
Jul 26, 2022

Conversation

taloric
Copy link
Contributor

@taloric taloric commented Jul 22, 2022

Description:
Add more extra information from SkyWalking ref to link attributes.
(It's the continuation fix after #11562 and this comment. )

Testing: No changes.

Documentation: No changes.

@taloric taloric requested review from a team and Aneurysm9 July 22, 2022 17:11
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

pinging @liqiangz as code owner

@taloric
Copy link
Contributor Author

taloric commented Jul 23, 2022

@TylerHelmuth
Besides, am I miss something ? I think @JaredTan95 maybe the code owner marked in https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/.github/CODEOWNERS#L183 :)

@TylerHelmuth
Copy link
Member

@TylerHelmuth Besides, am I miss something ? I think @JaredTan95 maybe the code owner marked in https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/.github/CODEOWNERS#L183 :)

You are correct, I accidentally pinged the exporter's owner.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

I see all existing tests are passing, but do any tests need updated because of the new mappings? Is there a tests that exists that should now also be testing that TraceId, ParentTraceSegmentId, and ParentSpanId are mapped correctly?

@taloric
Copy link
Contributor Author

taloric commented Jul 26, 2022

I see all existing tests are passing, but do any tests need updated because of the new mappings? Is there a tests that exists that should now also be testing that TraceId, ParentTraceSegmentId, and ParentSpanId are mapped correctly?

No... there are no tests needed updated in this pr. The existing TestSwReferencesToSpanLinks test covers the modification, it just added data passing through attributes and make no changes to the existing rules. Also the TraceId/ParentTraceSegmentId/ParentSpanId may mapped between different spans, I think there may not be a persuasive test in "pre-build data mapping".

As a replacement, I do an integration testing and I get the result which it's my expectation

I get export data with otlphttpexporter
sw-parent-segment-test

then I find the parent span which has SegmentId and SpanId related to the child in Jaeger:

sw-showcase-test

@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Jul 26, 2022
@TylerHelmuth
Copy link
Member

Adding ready-to-merge since the code owner has also approved.

@codeboten codeboten merged commit 460739f into open-telemetry:main Jul 26, 2022
@taloric taloric deleted the skywalkingreceiver-fix2 branch July 28, 2022 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants