Skip to content

feat(crypto-ffi): Add per-level log methods to Logger callback interface - #6972

Open
morlinbrot wants to merge 3 commits into
matrix-org:mainfrom
morlinbrot:main
Open

morlinbrot wants to merge 3 commits into
matrix-org:mainfrom
morlinbrot:main

Conversation

@morlinbrot

@morlinbrot morlinbrot commented Sep 3, 2026

Copy link
Copy Markdown

Closes #1759

The Logger trait previously only supported one catch-all log method which discarded the log level of tracing events. This adds dedicated per-level log methods to the trait.

LoggerWrapper has a new internal level field that tracks the log level, it's instantiated with Level::DEBUG as a default.

NOTE: This is a breaking change to the crate's public bindings API contract. I did not find any special instructions for cases like this but I wanted to note it prominently.

  • I've documented the public API changes in the appropriate changelog files (see Writing changelog entries).
  • This PR was made with the help of AI.

The Logger trait previously only supported one catch-all log method which discarded the log level of tracing events. This adds dedicated per-level log methods to the trait.

LoggerWrapper has a new internal level field that tracks the log level, it's instantiated with Level::DEBUG as a default

Signed-off-by: morlinbrot <morlinbrot@mailbox.org>
@morlinbrot
morlinbrot requested a review from a team as a code owner September 3, 2026 12:55
@morlinbrot
morlinbrot requested review from poljar and removed request for a team September 3, 2026 12:55
Signed-off-by: morlinbrot <morlinbrot@mailbox.org>
@codspeed

codspeed Bot commented Sep 3, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing morlinbrot:main (ecd1eba) with main (40f1b54)

Open in CodSpeed

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.23%. Comparing base (7a2a899) to head (ecd1eba).
⚠️ Report is 188 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6972   +/-   ##
=======================================
  Coverage   90.22%   90.23%           
=======================================
  Files         408      408           
  Lines      117273   117273           
  Branches   117273   117273           
=======================================
+ Hits       105814   105819    +5     
+ Misses       7519     7514    -5     
  Partials     3940     3940           

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

@morlinbrot

Copy link
Copy Markdown
Author

Pinging @Hywan since you reviewed a previously rejected PR and this implementation closely follows what you laid out in your comments there.

Two notes on decisions I made:

  • The above mentioned PR was criticized for removing the log method from Logger but that seems to be a legitimate change to me since its only previous user, LoggerWrapper's Write impl, doesn't use it anymore. And since we're already breaking the trait by splitting into multiple methods, I don't think it makes much sense to keep it for backwards compatibility either. Let me know if I'm overlooking something here.

  • The original ticket requests a data: String parameter be added to the logging methods. I deliberately left this out of the PR because the only current user of the logger is tracing_subscriber::fmt() which does not pass a data argument. Making use of one could be supported by a custom tracing_subscriber::Layer implementation but that felt like too big of a scope increase for this PR. Happy to look into this if it's wanted.

@poljar poljar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of interest is this something you need or are you adding it because we have an issue for it?

@@ -0,0 +1 @@
Changed the Logger callback interface to support per-level log methods, replacing its single catch-all log method

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please mark this as a breaking change by adding [**breaking**] to the start of the fragment.

Signed-off-by: morlinbrot <morlinbrot@mailbox.org>
@morlinbrot

Copy link
Copy Markdown
Author

Out of interest is this something you need or are you adding it because we have an issue for it?

I was exploring the code base and came across the issues marked "good first issue". Though that's something I could do real quick.

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.

Extend the logging callback interface in matrix-sdk-crypto-ffi

2 participants