Enhance Trace Context machenism. #20

jmjoy · 2022-06-05T05:19:02Z

Get current time as milliseconds rather than seconds.
Fix parent span id.
Add method to modify internal span object.

wu-sheng · 2022-06-05T12:49:56Z

src/context/trace_context.rs


        let mut span = Box::new(Span::new(
            self.next_span_id,
+            -1,


Why hard code the span parent Span ID? Parent span ID should be the previous unfinished span in this tracing context.

What is the mean of unfinished span? I think the left is correct, and right is incorrect.

Left Right

If /status/500 fails, and retry /status/400 again, then yes, the right is correct.
But this example seems not relative to why hardcode parent span ID? The right logic is, parent span ID should be the latest unfinished span ID in this context/thread.
I don't mean the original codes are correct, I don't check so carefully. But the new one seems not correct at least.

I think I understand what you mean. In this case, the parent span ID should be set to the Span ID of the previous span that has not set end time. The code will change a lot.

I think you just need to add a current span ref in TracingContext, and always use it as parent span ID, unless a parent Span ref is given as new span's parameter.
The current span in TracingContext should be set to its parent when it is closed.

In the Java agent, we use a stack(implement through a LinkedList) to push/pop the active span.
You could take this as a good reference,

https://github.com/apache/skywalking-java/blob/372d75403053c1eb81d3cc4ec696a675fab1da71/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/TracingContext.java#L80

I can see you did a lot of work on making rust-sdk in PHP ecosystem, and also helped our php-agent in skyapm org.
If you are willing to take more responsibility to this repo, this would be a very good start.

@Shikugawa implemented this for the first time, we have a lot to do to polish.

wu-sheng · 2022-06-05T12:50:09Z

src/context/trace_context.rs


        let span = Box::new(Span::new(
            self.next_span_id,
+            0,


Why initial as 0?

wu-sheng · 2022-06-08T13:00:06Z

Any update?

jmjoy · 2022-06-09T00:51:17Z

Any update?

No.

jmjoy · 2022-06-10T10:40:43Z

Is this solution OK? If possible, I will fix CI.

wu-sheng · 2022-06-10T10:44:29Z

Is this solution OK? If possible, I will fix CI.

What solution? I left comments days ago. Wait for your reply.

jmjoy · 2022-06-10T14:14:54Z

Is this solution OK? If possible, I will fix CI.

What solution? I left comments days ago. Wait for your reply.

Sorry, I found that my comment is pending and I have to submit it. 😅

codecov-commenter · 2022-06-11T03:57:54Z

Codecov Report

Merging #20 (0fa7b1b) into master (d7baf4c) will increase coverage by 1.92%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   84.94%   86.86%   +1.92%     
==========================================
  Files           9        9              
  Lines         279      297      +18     
==========================================
+ Hits          237      258      +21     
+ Misses         42       39       -3

Impacted Files	Coverage Δ
src/context/propagation/context.rs	`100.00% <ø> (ø)`
src/context/system_time.rs	`0.00% <ø> (ø)`
src/context/trace_context.rs	`74.33% <90.00%> (+4.52%)`	⬆️
tests/trace_context.rs	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7baf4c...0fa7b1b. Read the comment docs.

jmjoy · 2022-06-11T07:08:41Z

I rewrite the logic of get the parent span id, please review.

wu-sheng · 2022-06-11T07:18:53Z

tests/trace_context.rs

+// pub mod skywalking_proto {
+//     pub mod v3 {
+//         tonic::include_proto!("skywalking.v3");
+//     }
+// }


Why do you comment these lines?

wu-sheng · 2022-06-11T07:21:39Z

The new one looks better, one questions
Do we expect trace context to only exist in one running thread? Just check, because officially, there are two concepts, (1) snapshot concept for across threads context propagation, and (2) async span mode for a span begins and finishes in two threads in other agent implementations.

Please fix e2e test.

jmjoy · 2022-06-13T03:05:37Z

The new one looks better, one questions Do we expect trace context to only exist in one running thread? Just check, because officially, there are two concepts, (1) snapshot concept for across threads context propagation, and (2) async span mode for a span begins and finishes in two threads in other agent implementations.

Please fix e2e test.

Fixed.

Due to the concurrency mechanism of trust, TracingContext and Span should be modified to support multi threading. I think they should be modified in the next issue.
There is no mechanism to ensure that the user calls finalize_span, I think it is better to implement Drop trait automatic call instead, and it can also be modified in the next issue.

wu-sheng · 2022-06-13T04:00:15Z

Both are making sense. About (1), let's talk in a discussion/issue in the main repo about the details, to make you easier.

jmjoy added 11 commits May 30, 2022 11:51

Update skywalking-data-collect-protocol to v9.0.0.

904681c

Update dependencies versions.

7236636

Disable tonic build server.

ad08f86

Remove Cargo.lock from version management.

4c97468

Fix CI failed.

4815763

Add set_component_id method for Span.

4bb963d

Add span_object_mut method of Span.

6b5e397

Modify time to millis.

987d182

Fix parent span id.

b551b66

Merge branch 'hack'

62f24ad

Revert build.rs.

7c61335

wu-sheng requested changes Jun 5, 2022

View reviewed changes

wu-sheng added the stale No update for days label Jun 9, 2022

Fix parent span id.

9c041a9

Add more test for create span.

0fa7b1b

wu-sheng reviewed Jun 11, 2022

View reviewed changes

Fix integration tests and propagation.

d25fb64

Fix crossprocess_test.

7fe1b2f

wu-sheng added this to the 0.2.0 milestone Jun 13, 2022

wu-sheng removed the stale No update for days label Jun 13, 2022

wu-sheng added the enhancement New feature or request label Jun 13, 2022

wu-sheng changed the title ~~Fix time and parent span id.~~ Enhance Trace Context machenism. Jun 13, 2022

wu-sheng approved these changes Jun 13, 2022

View reviewed changes

wu-sheng merged commit 5fded8e into apache:master Jun 13, 2022

jmjoy mentioned this pull request Jul 1, 2022

[Feature] About support multi threads and auto finalize for skywalking-rust. apache/skywalking#9297

Closed

3 tasks

Enhance Trace Context machenism. #20

Enhance Trace Context machenism. #20

Uh oh!

Conversation

jmjoy commented Jun 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Jun 8, 2022

Uh oh!

jmjoy commented Jun 9, 2022

Uh oh!

jmjoy commented Jun 10, 2022

Uh oh!

wu-sheng commented Jun 10, 2022

Uh oh!

jmjoy commented Jun 10, 2022

Uh oh!

codecov-commenter commented Jun 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jmjoy commented Jun 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Jun 11, 2022

Uh oh!

jmjoy commented Jun 13, 2022

Uh oh!

wu-sheng commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jun 11, 2022 •

edited

Loading