Skip to content

Conversation

@jmjoy
Copy link
Member

@jmjoy jmjoy commented May 19, 2023

Sometimes we need to create a span following by the last active span instead of as the a child span, like in https://github.com/apache/skywalking-php/blob/eabb210b42ff652b8e34d8d99a933b5489b80397/src/plugin/plugin_curl.rs#LL276C9-L276C9

  1. Add Span::prepare_for_async mehtod, which will return the AsyncSpan object, when it dropped, it will auto finished and notify TracingContext.
  2. Add TracingContext::wait method, which will block to wait all AsyncSpan finished.
  3. Add AbstractSpan trait, Span and AsyncSpan all implement it.

@jmjoy jmjoy added this to the 0.7.0 milestone May 19, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2023

Codecov Report

Merging #58 (d702feb) into master (632766f) will decrease coverage by 1.01%.
The diff coverage is 6.73%.

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
- Coverage   17.69%   16.69%   -1.01%     
==========================================
  Files          17       18       +1     
  Lines         565      629      +64     
==========================================
+ Hits          100      105       +5     
- Misses        465      524      +59     
Impacted Files Coverage Δ
src/logging/record.rs 0.00% <ø> (ø)
src/trace/span.rs 0.00% <0.00%> (ø)
src/trace/trace_context.rs 24.07% <9.09%> (-0.93%) ⬇️
src/common/wait_group.rs 15.38% <15.38%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jmjoy jmjoy marked this pull request as ready for review May 19, 2023 14:20
@jmjoy jmjoy requested a review from wu-sheng May 19, 2023 14:20
@wu-sheng
Copy link
Member

By following, you mean sharing the same parent span with the current active span? Two active spans created and running?

@jmjoy
Copy link
Member Author

jmjoy commented May 19, 2023

By following, you mean sharing the same parent span with the current active span? Two active spans created and running?

Yes, like curl_multi_*, many HTTP requests are requested simultaneously in a multi-threaded manner, so they should belong to the same parent. Is there a better way?

@wu-sheng
Copy link
Member

If they run concurrently, usually we prefer span created and change into async mode(not be active span anymore)

https://skywalking.apache.org/docs/skywalking-java/next/en/setup/service-agent/java-agent/java-plugin-development-guide/#async-span-apis

This is Java dev doc. Notice, Java trace segment is in one thread only, so, when thread crossing happens, you need to go for new segment. But then the span duration can't represent the HTTP call anymore. So, we need async span to create in thread A, but finish in thread B. This concept should fit your scenarios as well.

@wu-sheng
Copy link
Member

follow up span was introduced by Opentracing, but very confusing. So, OTEL doesn't mention that, SkyWalking never have this concept too.

@jmjoy jmjoy marked this pull request as draft May 20, 2023 08:59
@jmjoy jmjoy changed the title Add TracingContext::create_following_*_span methods. Add Span::prepare_for_async method and AbstractSpan trait. May 20, 2023
@jmjoy jmjoy marked this pull request as ready for review May 20, 2023 15:01
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Please update doc about new APIs.

@jmjoy
Copy link
Member Author

jmjoy commented May 23, 2023

Please update doc about new APIs.

The documentation for this part of the API has already been mentioned in the document comments (generated in docs.rs), and mentioning it on readme will make it very large?

@wu-sheng
Copy link
Member

Yes, such as how to typically use these new APIs.

@jmjoy jmjoy merged commit 22aa373 into apache:master May 23, 2023
@jmjoy jmjoy deleted the span branch May 23, 2023 10:25
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.

3 participants