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

ffi: use fork of tracing crate with a fix for Android logs rotation #4038

Merged

Conversation

jmartinesp
Copy link
Contributor

@jmartinesp jmartinesp commented Sep 26, 2024

The tracing library has an issue that prevents the configured log rotation from working on Android. While there has been a proposed fix for some time, it hasn't been merged yet.

We recently discovered some cases of users who had hundreds of log files, several times our current max limit, and today I confirmed old logs weren't being deleted.

My proposed solution is to use a forked tracing library in the meantime, along with a forked paranoid-android one, which needs to use this former fork.

I tested this on an Android client and I saw the old logs being removed, as expected.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@jmartinesp jmartinesp requested a review from a team as a code owner September 26, 2024 14:08
@jmartinesp jmartinesp requested review from poljar and removed request for a team September 26, 2024 14:08
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.51%. Comparing base (322c5b3) to head (10a0b7e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4038      +/-   ##
==========================================
+ Coverage   84.50%   84.51%   +0.01%     
==========================================
  Files         266      266              
  Lines       28475    28475              
==========================================
+ Hits        24062    24066       +4     
+ Misses       4413     4409       -4     

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

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Please use the patch section for this like we already do for some other things:

matrix-rust-sdk/Cargo.toml

Lines 122 to 124 in 322c5b3

[patch.crates-io]
async-compat = { git = "https://github.com/jplatte/async-compat", rev = "16dc8597ec09a6102d58d4e7b67714a35dd0ecb8" }
const_panic = { git = "https://github.com/jplatte/const_panic", rev = "9024a4cb3eac45c1d2d980f17aaee287b17be498" }
.

Since this is only needed for the bindings, there's no need to change the dependency definition. This way we'll be able to publish releases, while Android and the bindings, which don't need to be published on crates.io, can use the fork.

@jmartinesp jmartinesp requested a review from poljar September 26, 2024 14:44
@jmartinesp
Copy link
Contributor Author

I think it should be ok after e7f5135

tracing-core = { git = "https://github.com/element-hq/tracing.git", rev = "ca9431f74d37c9d3b5e6a9f35b2c706711dab7dd" }
tracing-subscriber = { git = "https://github.com/element-hq/tracing.git", rev = "ca9431f74d37c9d3b5e6a9f35b2c706711dab7dd" }
tracing-appender = { git = "https://github.com/element-hq/tracing.git", rev = "ca9431f74d37c9d3b5e6a9f35b2c706711dab7dd" }
paranoid-android = { git = "https://github.com/element-hq/paranoid-android.git", rev = "69388ac5b4afeed7be4401c70ce17f6d9a2cf19b" }
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a newline missing here at the end.

@jmartinesp jmartinesp force-pushed the misc/use-fork-of-tracing-to-fix-android-log-rotation branch from e7f5135 to 10a0b7e Compare September 27, 2024 07:59
@jmartinesp jmartinesp enabled auto-merge (rebase) September 27, 2024 08:00
@jmartinesp jmartinesp merged commit 9b7f89c into main Sep 27, 2024
40 checks passed
@jmartinesp jmartinesp deleted the misc/use-fork-of-tracing-to-fix-android-log-rotation branch September 27, 2024 08:37
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