feat(ffi): move tracing setup from the final client to the ffi layer #4492

stefanceriu · 2025-01-09T11:19:55Z

Having the final clients define the tracing filters / log levels proved to be tricky to keep in check resulting missing logs (e.g. recently introduced modules like the event cache) or unexpected behaviors (e.g. missing panics because we don't set a global filter). As such we decided to move the tracing setup and default definitions over to the rust side and let rust developers have full control over them.

We will now take a general log level and the final client's target name and apply them on top of the rust side defined default. Targets that log more than the requested by default will remain unchanged while the others will increase their log levels to match. Certain targets like hyper will be ignored in this step as they're too verbose.

BillCarsonFr · 2025-01-09T11:26:51Z

bindings/matrix-sdk-ffi/src/platform.rs

@@ -245,12 +231,65 @@ pub struct TracingFileConfiguration {
    max_files: Option<u64>,
 }

+#[derive(PartialEq, PartialOrd)]
+enum LogTarget {


I think EXA has more target than that https://github.com/element-hq/element-x-android/blob/65ce91a8fbe287564bbdaa969ce9b925edacdb6c/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/tracing/TracingFilterConfiguration.kt#L48-L66

Would be nice to have a CustomTarget(rawString)? that would allow to add a new target wihtout having to compile a custom sdk when developping?

I think EXA has more target than that

Seems so. The missing ones are matrix_sdk_ffi::uniffi_api , matrix_sdk and matrix_sdk::send_queue. Not sure if we should include them or not to be honest ¯_(ツ)_/¯ Perhaps keep matrix_sdk on info and not add the rest?

Would be nice to have a CustomTarget(rawString)?

Thought about exposing an optional filter parameter that will just override everything but I figured it's best we keep the api simple as a first step. I'm not sure how often you'll want to debug something with custom tracing on the app side without having a custom sdk version as well, moment in which tweaking the logs becomes easy.

Thought about exposing an optional filter parameter that will just override everything but I figured it's best we keep the api simple as a first step. I'm not sure how often you'll want to debug something with custom tracing on the app side without having a custom sdk version as well, moment in which tweaking the logs becomes easy.

Could the TracingConfiguration include an extra_logs: Vec<(String, LogLevel)>, that would be applied as such without any checks? Having some dynamic way to set some logs is definitely a recurring need, and will pay off super quick.

Then we wouldn't need the mandatory app_target field in there, as it could be part of the extra logs. (And that gives you the possibility to use different spans if you'd like!)

Oh yeah, nice idea, let's do it! 💪

Okay, so I had a play and ended up with passing in only an array of targets as opposed to (target, level) as doing the latter would force the final client to duplicate the level adjustment logic done here.

If we pass in only the targets we can keep using the same logic while offering just enough flexibility imho.

codecov · 2025-01-09T11:34:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.36%. Comparing base (df4b696) to head (36500c6).
Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4492      +/-   ##
==========================================
- Coverage   85.38%   85.36%   -0.02%     
==========================================
  Files         284      284              
  Lines       31885    31885              
==========================================
- Hits        27225    27220       -5     
- Misses       4660     4665       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Having the final clients define the tracing filters / log levels proved to be tricky to keep in check resulting missing logs (e.g. recently introduced modules like the event cache) or unexpected behaviors (e.g. missing panics because we don't set a global filter). As such we decided to move the tracing setup and default definitions over to the rust side and let rust developers have full control over them. We will now take a general log level and the final client's target name and apply them on top of the rust side defined default. Targets that log more than the requested by default will remain unchanged while the others will increase their log levels to match. Certain targets like `hyper` will be ignored in this step as they're too verbose.

…from the final client to the rust side. - relates to matrix-org/matrix-rust-sdk#4492

bnjbvr

Thanks a bunch for doing this, you rock! My only blocking comment is about extra_logs, the rest are mostly nits.

bindings/matrix-sdk-ffi/src/platform.rs

bnjbvr · 2025-01-09T12:23:25Z

bindings/matrix-sdk-ffi/src/platform.rs

@@ -245,12 +231,65 @@ pub struct TracingFileConfiguration {
    max_files: Option<u64>,
 }

+#[derive(PartialEq, PartialOrd)]
+enum LogTarget {


Thought about exposing an optional filter parameter that will just override everything but I figured it's best we keep the api simple as a first step. I'm not sure how often you'll want to debug something with custom tracing on the app side without having a custom sdk version as well, moment in which tweaking the logs becomes easy.

Could the TracingConfiguration include an extra_logs: Vec<(String, LogLevel)>, that would be applied as such without any checks? Having some dynamic way to set some logs is definitely a recurring need, and will pay off super quick.

Then we wouldn't need the mandatory app_target field in there, as it could be part of the extra logs. (And that gives you the possibility to use different spans if you'd like!)

bindings/matrix-sdk-ffi/src/platform.rs

bnjbvr · 2025-01-09T12:25:17Z

bindings/matrix-sdk-ffi/src/platform.rs

+            LogTarget::MatrixSDKBaseSlidingSync => "matrix_sdk_base::sliding_sync",
+            LogTarget::MatrixSDKUITimeline => "matrix_sdk_ui::timeline",
+            LogTarget::MatrixSDKEventCache => "matrix_sdk::event_cache",
+            LogTarget::MatrixSDKEventCacheStore => "matrix_sdk_sqlite::event_cache_store",


For the event cache pack, we'd need also LogTarget::MatrixSdkBaseEventCache. But I can do that as a follow-up, if you'd like :)

I've addedMatrixSdkBaseEventCache that defaults to Info like the others and I've also added an matrix_sdk wide one that also defaults to info but won't drop lower than that.

bindings/matrix-sdk-ffi/src/platform.rs

…dkBaseEventCache`

…from the final client to the rust side. - relates to matrix-org/matrix-rust-sdk#4492

bnjbvr

🥳

poljar · 2025-01-09T19:56:49Z

I think that this should have been merged at the same time matrix-org/complement-crypto#159 got merged, now our CI is busted 😢.

stefanceriu · 2025-01-10T07:32:45Z

Bah, didn't even cross my mind 😬
Perhaps we should be more leninent on complement crypto as it's an external project.

…from the final client to the rust side. - relates to matrix-org/matrix-rust-sdk#4492

poljar · 2025-01-10T08:10:10Z

Well complement-crypto was designed with this in mind and it did work out nicely in the past, I don't think we need to change this.

stefanceriu · 2025-01-10T08:12:32Z

Okay then perhaps we should not try to match the branches and instead always run complement crypto on main so it blocks PRs?

poljar · 2025-01-10T08:27:58Z

The matching branches are here so you can develop the complement changes and double check that your changes in the SDK and in complement crypto pass the CI.

It's a chicken and egg problem, if we run always against main of complement crypto then we're never able to get the CI green in the PR. Do note, if we merge the complement crypto PR and forget to merge the SDK PR, then we also get a CI failure.

Sadly, I don't see any better way of doing this.

stefanceriu · 2025-01-10T08:32:55Z

Do note, if we merge the complement crypto PR and forget to merge the SDK PR, then we also get a CI failure.

Ah right, didn't think about that. Well I guess we'll just have to do to the best with what we have.

stefanceriu requested a review from a team as a code owner January 9, 2025 11:19

stefanceriu requested review from Hywan and removed request for a team January 9, 2025 11:19

stefanceriu mentioned this pull request Jan 9, 2025

Move tracing configuration to the rust side. element-hq/element-x-ios#3668

Merged

BillCarsonFr reviewed Jan 9, 2025

View reviewed changes

stefanceriu force-pushed the stefan/ffiTracingConfiguration branch from dcfc9d0 to 33e277f Compare January 9, 2025 11:38

stefanceriu added a commit to matrix-org/complement-crypto that referenced this pull request Jan 9, 2025

Fix tracing configuration following API changes - move configuration …

8315089

…from the final client to the rust side. - relates to matrix-org/matrix-rust-sdk#4492

bnjbvr requested changes Jan 9, 2025

View reviewed changes

stefanceriu added 4 commits January 9, 2025 15:01

Fix typos and improve comments

68d5c48

Rename log targets.

c84ae8e

Switch from passing in the app_target to a vector of extra targets.

4fb5bcd

Add a default immutable matrix_sdk wide log target and the `MatrixS…

36500c6

…dkBaseEventCache`

stefanceriu added a commit to matrix-org/complement-crypto that referenced this pull request Jan 9, 2025

Fix tracing configuration following API changes - move configuration …

4e79db9

…from the final client to the rust side. - relates to matrix-org/matrix-rust-sdk#4492

stefanceriu requested a review from bnjbvr January 9, 2025 15:12

bnjbvr approved these changes Jan 9, 2025

View reviewed changes

stefanceriu merged commit c4bfbd0 into main Jan 9, 2025
40 checks passed

stefanceriu deleted the stefan/ffiTracingConfiguration branch January 9, 2025 16:08

stefanceriu mentioned this pull request Jan 9, 2025

Fix tracing configuration following API changes - move configuration … matrix-org/complement-crypto#159

Merged

poljar pushed a commit to matrix-org/complement-crypto that referenced this pull request Jan 10, 2025

Fix tracing configuration following API changes - move configuration …

4ba6c4a

…from the final client to the rust side. - relates to matrix-org/matrix-rust-sdk#4492

ganfra mentioned this pull request Jan 16, 2025

update rust sdk 0.2.75 element-hq/element-x-android#4158

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ffi): move tracing setup from the final client to the ffi layer #4492

feat(ffi): move tracing setup from the final client to the ffi layer #4492

stefanceriu commented Jan 9, 2025

BillCarsonFr Jan 9, 2025

stefanceriu Jan 9, 2025

bnjbvr Jan 9, 2025

stefanceriu Jan 9, 2025

stefanceriu Jan 9, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

bnjbvr left a comment

bnjbvr Jan 9, 2025

bnjbvr Jan 9, 2025

stefanceriu Jan 9, 2025

bnjbvr left a comment

poljar commented Jan 9, 2025

stefanceriu commented Jan 10, 2025

poljar commented Jan 10, 2025

stefanceriu commented Jan 10, 2025

poljar commented Jan 10, 2025

stefanceriu commented Jan 10, 2025

feat(ffi): move tracing setup from the final client to the ffi layer #4492

feat(ffi): move tracing setup from the final client to the ffi layer #4492

Conversation

stefanceriu commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

poljar commented Jan 9, 2025

stefanceriu commented Jan 10, 2025

poljar commented Jan 10, 2025

stefanceriu commented Jan 10, 2025

poljar commented Jan 10, 2025

stefanceriu commented Jan 10, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading