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/skywalking] Fix skywalking traceid and spanid convertion. #11562

Merged
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 review from a team and 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

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

  • 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

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


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.

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.

@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.

CHANGELOG.md Outdated Show resolved Hide resolved
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

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
@taloric taloric deleted the skywalkingreceiver-fix branch July 22, 2022 09:44
codeboten pushed a commit that referenced this pull request Jul 26, 2022
…ng ref. (#12651)

Add more extra information from SkyWalking ref to link attributes.
(It's the continuation fix after #11562 and this comment. )
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants