Add tracer.#26
Conversation
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
===========================================
- Coverage 86.91% 75.29% -11.63%
===========================================
Files 9 10 +1
Lines 298 344 +46
===========================================
Hits 259 259
- Misses 39 85 +46
Continue to review full report at Codecov.
|
wu-sheng
left a comment
wu-sheng
left a comment
There was a problem hiding this comment.
Usually, this concept in SDK should be called TRACER, rather than ContextManager.
I used ContextManager in Java as we have a tracer manual API, so I don't want to create a duplicated one.
go2sky(golang SDK) is using this concept, https://github.com/SkyAPM/go2sky#quickstart.
| use tokio; | ||
|
|
||
| async fn handle_request(reporter: ContextReporter) { | ||
| async fn handle_request(reporter: Reporter) { |
There was a problem hiding this comment.
I think we should add a new API's doc here.
There was a problem hiding this comment.
Maybe we can integrate Tracer and Reporter, like in go.
There was a problem hiding this comment.
Yes, agree.
And whether they should be const, most likely user's choice, right?
We don't have to require that.
|
Is this API OK? If yes, I continue to modify e2e. |
wu-sheng
left a comment
wu-sheng
left a comment
There was a problem hiding this comment.
Mostly good, one thing to be uncertain.
| tokio::spawn(handle_request(tracer.clone())); | ||
|
|
||
| // Start server... | ||
| // Block to report. |
There was a problem hiding this comment.
What does block to report work for?
There was a problem hiding this comment.
What does block to report work for?
Because dead loop to receive segment to collect, and wait a shutdown signal, will block.
There was a problem hiding this comment.
How about changing this to
Open reporter as daemon thread(or something more precious in rust) to run.
Block seems a negative word.
WDYT?
There was a problem hiding this comment.
Already collect as daemon, the block is waiting shutdown signal, otherwise the graceful shutdown cannot be done.
|
The latest one is clear and more elegant. Thanks. Let's fix e2e. |
|
E2e fixed. |
wu-sheng
added
documentation
| futures-core = "0.3.21" | ||
| futures-util = "0.3.21" | ||
| prost = "0.10.4" | ||
| prost-derive = "0.10.1" | ||
| thiserror = "1.0.31" | ||
| tokio = { version = "1.18.2", features = ["full"] } | ||
| tonic = "0.7.2" | ||
| tonic = { version = "0.7.2", features = ["codegen"] } | ||
| tracing = "0.1.35" |
There was a problem hiding this comment.
Why aren't these new dependencies detected by skywalking eyes? I think these are new dependencies, no?
There was a problem hiding this comment.
These dependencies actually already exist (required by the dependencies explicitly declared in Cargo.toml ), I just re-import the crate name to use, run Cargo tree can see the dependencies tree.
|
Line 23 in 4339384 We forgot to update this to 2022 in the last release. |
No, the edition is three-year period, such as 2015, 2018, 2021. |
|
Is this a rust specific edition rule? |
Edition is a big concept of rust, used for syntax breaking compatible, ref: https://doc.rust-lang.org/edition-guide/editions/index.html. |
3 participants
Add the
Tracer, integrate withReporter, like in go.