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

Jump to bottom

[receiver/skywalking] Fix skywalking traceid and spanid convertion. #11562

Merged
merged 8 commits into from
Jul 14, 2022
Merged

[receiver/skywalking] Fix skywalking traceid and spanid convertion. #11562

merged 8 commits into from
Jul 14, 2022

Conversation

taloric
Copy link
Contributor

@taloric taloric commented Jun 27, 2022

Description:

  • Backgrounds:

    • Skywalking use segments to cover spans as a group. Also SegmentId + SpanId to identify an unique span in TraceData.
    • Protocols could be found here.
    • The Skywalking receive server handle one segment object in each http request. Those segments have different segmentId and share the same traceId.

  • Code Changes:

    • Use a new rule to convert Skywalking-TraceId to Opentelemetry-TraceId. (The old rules breaks relations between segments because it use pointer of strings to generate a new traceId.)
      • Skywalking-TraceId format be like: de5980b8-fce3-4a37-aab9-b4ac3af7eedd(RFC4122) or 56a5e1c519ae4c76a2b8b11d92cead7f.1.16563474296430001.
    • Use SegmentId in the convertion of SpanId to make sure the SpanId is globally unique.
      • SegmentId format be like: 4f2f27748b8e44ecaf18fe0347194e86.33.16560607369950066.
      • The first part of SegmentId(4f2f27748b8e44ecaf18fe0347194e86) will repeat multiple times in a TraceData, so the last part is the real unique identifier of segments.

Testing:

  • Add Test_stringToTraceID(): Make sure Skywalking-TraceId convert to the Otel-TraceId by a rule.
  • Add Test_segmentIdToSpanId(): Using Skywalking segmentId & spanId to generate a new Otel-SpanId.

Documentation:
No changes.

@taloric taloric requested a review from a team as a code owner June 27, 2022 16:51
@taloric taloric requested a review from Aneurysm9 June 27, 2022 16:51
@taloric taloric force-pushed the skywalkingreceiver-fix branch 2 times, most recently from a8e349b to e278956 Compare June 27, 2022 17:00
@taloric
Copy link
Contributor Author

taloric commented Jun 27, 2022

  • Add Jaeger test result here (with otel agent) :
    skywalking-jaeger

  • Same application trace in Skywalking dashboard (with skywalking agent) :
    skywalking-form2

codeboten
codeboten reviewed Jun 27, 2022
View reviewed changes
Copy link
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.

Pinging code owner @JaredTan95, please review

@taloric
Copy link
Contributor Author

taloric commented Jun 28, 2022

@JaredTan95
I found that Skywalking traceid has 2 formats , like de5980b8-fce3-4a37-aab9-b4ac3af7eedd or 56a5e1c519ae4c76a2b8b11d92cead7f.1.16563474296430001 ( Skywalking Live Demo ), but even in protocols I can find no descriptions about that . Wondering if you have any idea of this...

@taloric taloric force-pushed the skywalkingreceiver-fix branch 2 times, most recently from 8c977bc to 539c06f Compare June 28, 2022 14:19
@JaredTan95
Copy link
Member

JaredTan95 commented Jun 29, 2022 •
edited
Loading

  • I found that Skywalking traceid has 2 formats , like de5980b8-fce3-4a37-aab9-b4ac3af7eedd or

de5980b8-fce3-4a37-aab9-b4ac3af7eedd format is produced by the browser/js SDK or envoy/nginx-lua sdk.

@JaredTan95
Copy link
Member

  • Add Jaeger test result here (with otel agent) :
    skywalking-jaeger
  • Same application trace in Skywalking dashboard (with skywalking agent) :
    skywalking-form2

hi, @taloric Thanks for the fix, I don't see that your screenshot involves using the skywalking agent to send tracing data to otel collector skywalking receiver and exporter into jaeger. Can you update one?

@taloric
Copy link
Contributor Author

taloric commented Jul 1, 2022 •
edited
Loading

hi, @taloric Thanks for the fix, I don't see that your screenshot involves using the skywalking agent to send tracing data to otel collector skywalking receiver and exporter into jaeger. Can you update one?

@JaredTan95 Thanks for the review. I did find some bugs during test with skywalking agent. I make some changes about traceid convertion rules.

  • Here are my test result (send trace data with skywalking-java-agent)

  • They build the same results:

  • Jaeger:
    jaeger-java-agent-test

  • skywalking:
    skywalking-java-agent

JaredTan95
JaredTan95 approved these changes Jul 1, 2022
View reviewed changes
Copy link
Member

@JaredTan95 JaredTan95 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

good catches, LGTM
@taloric BTW, can you change the issue title to [receiver/skywalking] Fix skywalking traceid and spanid convertion. ?

@JaredTan95
Copy link
Member

@codeboten PTAL on behalf of approvers.

@JaredTan95 JaredTan95 mentioned this pull request Jul 2, 2022
Closed
@taloric taloric changed the title Fix skywalking traceid and spanid convertion. [receiver/skywalking] Fix skywalking traceid and spanid convertion. Jul 2, 2022
@taloric
Copy link
Contributor Author

taloric commented Jul 6, 2022

@codeboten @JaredTan95 @sharang
Refactor the code because there are too much magic numbers which are not understandable. Please take a look.

JaredTan95
JaredTan95 reviewed Jul 8, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
JaredTan95
JaredTan95 approved these changes Jul 11, 2022
View reviewed changes
Copy link
Member

@JaredTan95 JaredTan95 left a comment

Choose a reason for hiding this comment

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

LGTM
thx for your contributions

@JaredTan95
Copy link
Member

@Aneurysm9 PTAL on behalf of approvers.

@taloric
Copy link
Contributor Author

taloric commented Jul 12, 2022

Skywalking Agent will write skywalking trace info in logs like this:
image
But after conversion, the relationship between trace and log were lost. So I add skywalking source information in attributes.

@Aneurysm9 @codeboten @JaredTan95 please review.

@JaredTan95
Copy link
Member

So I add skywalking source

This is another bug or caused by your pr?

@taloric
Copy link
Contributor Author

taloric commented Jul 13, 2022 •
edited
Loading

So I add skywalking source

This is another bug or caused by your pr?

It's not caused by this PR. And It's aim to solve the same issue.

I think what our expectation is: application -> Trace data -> otel / skywalking is a full Trace. We can track any step in this Trace with traceid or spanid.

This PR aim to solve Trace broken issues when skywalking traceid convert to otel traceid. (which was done in first commit. ) But that didn't build relations between application Trace logs and otel Trace export result.

So I add skywalking original traceid as attributes to connect application logs with otel export results. Now you can get any skywalking traceid in application logs, and find the same Trace directly in Jaeger or other backends.

@JaredTan95

@JaredTan95
Copy link
Member

got it. make sense, LGTM

@taloric
Copy link
Contributor Author

taloric commented Jul 14, 2022

@codeboten @Aneurysm9 @bogdandrutu would you please take a look at this PR?

@codeboten codeboten merged commit f1666ca into open-telemetry:main Jul 14, 2022
atoulme pushed a commit to atoulme/opentelemetry-collector-contrib that referenced this pull request Jul 16, 2022
@taloric @atoulme
[receiver/skywalking] Fix skywalking traceid and spanid convertion. (o…
9363a9d 
…pen-telemetry#11562)

* Fix skywalking traceid and spanid convertion.

* Update CHANGELOG.md

* use uuid.Parse instead of hextable to convert id

* [receiver/skywalking] fix skywalking-rcv tid convertion

* use uuid.Parse instead of hextable to convert id

* [receiver/skywalking] fix skywalking-rcv tid convertion

* [receiver/skywalking] Add skywalking attributes into tracedata

Co-authored-by: Alex Boten <[email protected]>
@taloric taloric deleted the skywalkingreceiver-fix branch July 22, 2022 09:44
@taloric taloric mentioned this pull request Jul 22, 2022
Merged
codeboten pushed a commit that referenced this pull request Jul 26, 2022
@taloric
[receiver/skywalkingreceiver] Add extra link attributes from skywalki…
460739f 
…ng ref. (#12651)

Add more extra information from SkyWalking ref to link attributes.
(It's the continuation fix after #11562 and this comment. )
ag-ramachandran referenced this pull request in ag-ramachandran/opentelemetry-collector-contrib Sep 15, 2022
@taloric @ag-ramachandran
[receiver/skywalking] Fix skywalking traceid and spanid convertion. (#…
8386748 
…11562)

* Fix skywalking traceid and spanid convertion.

* Update CHANGELOG.md

* use uuid.Parse instead of hextable to convert id

* [receiver/skywalking] fix skywalking-rcv tid convertion

* use uuid.Parse instead of hextable to convert id

* [receiver/skywalking] fix skywalking-rcv tid convertion

* [receiver/skywalking] Add skywalking attributes into tracedata

Co-authored-by: Alex Boten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten approved these changes

@sharang sharang sharang approved these changes

@JaredTan95 JaredTan95 JaredTan95 approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9 Aneurysm9 is a code owner automatically assigned from open-telemetry/collector-contrib-approvers

Assignees

@bogdandrutu bogdandrutu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@taloric @JaredTan95 @codeboten @sharang @bogdandrutu