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

Add a root span to the client #1447

Merged
merged 4 commits into from
Feb 16, 2023
Merged

Add a root span to the client #1447

merged 4 commits into from
Feb 16, 2023

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Feb 3, 2023

This allows us to have a nice single tree view of the whole client run instead of having traces scattered around:

image

Not all methods have been handled, and for some methods we would need to apply the root as a parent only if there isn't a previous parent, but this should be a good start.

Requires: #1446

crates/matrix-sdk/src/client/builder.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/client/login_builder.rs Show resolved Hide resolved
Comment on lines +2177 to 2188
#[instrument(skip_all, parent = &self.root_span)]
pub async fn sync_with_callback<C>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a method that gets called from other sync methods. Using parent = here will make that relation invisible in logs, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's called from the sync() method, which doesn't create a span itself.

crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
@poljar
Copy link
Contributor Author

poljar commented Feb 3, 2023

One of the problems here is that our root span ~never gets uploaded. My comment that we can drop the span and get it emitted is false.

The spans end up in a neat tree, but the root one is missing and there are warnings about an invalid parent id.

This seems to be related: open-telemetry/opentelemetry-rust#888.

@poljar poljar force-pushed the poljar/sliding-sync-stream-refactor branch from 7e9187e to 12d7a7f Compare February 3, 2023 21:32
Base automatically changed from poljar/sliding-sync-stream-refactor to main February 7, 2023 13:26
@poljar
Copy link
Contributor Author

poljar commented Feb 13, 2023

This PR proved that we want to have a root span, though I don't think a library should create root spans. What we instead want is to let EX create a root span and have that either be automatically propagated over the FFI or let the ClientBuilder take a root span.

Will happen in a different PR.

@poljar poljar closed this Feb 13, 2023
@poljar poljar reopened this Feb 16, 2023
@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Base: 74.43% // Head: 74.54% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (bef9243) compared to base (7cc06cb).
Patch coverage: 85.24% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1447      +/-   ##
==========================================
+ Coverage   74.43%   74.54%   +0.10%     
==========================================
  Files         124      124              
  Lines       13841    13881      +40     
==========================================
+ Hits        10303    10348      +45     
+ Misses       3538     3533       -5     
Impacted Files Coverage Δ
crates/matrix-sdk/src/sliding_sync.rs 0.00% <ø> (ø)
crates/matrix-sdk/src/room/joined.rs 65.64% <81.48%> (+2.48%) ⬆️
crates/matrix-sdk/src/client/mod.rs 88.75% <82.60%> (+0.73%) ⬆️
crates/matrix-sdk/src/client/builder.rs 76.98% <100.00%> (+1.56%) ⬆️
crates/matrix-sdk/src/client/login_builder.rs 26.35% <100.00%> (+0.57%) ⬆️
crates/matrix-sdk/src/room/timeline/mod.rs 80.34% <100.00%> (ø)
crates/matrix-sdk/src/room/timeline/inner.rs 67.95% <0.00%> (+0.11%) ⬆️
crates/matrix-sdk-crypto/src/machine.rs 76.31% <0.00%> (+0.52%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@poljar poljar merged commit 9649100 into main Feb 16, 2023
@poljar poljar deleted the poljar/root-span branch February 16, 2023 14:31
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.

2 participants