-
-
Notifications
You must be signed in to change notification settings - Fork 594
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 option for MatrixClient.initRustCrypto() to disable tracing. #4462
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks ok to me, but I want to defer to the other reviewers as to whether this the right way to do this.
This error from the build looks genuine though:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes needed to fix the build
hey @andybalaam the error can be fixed in two ways:
|
Hi @andybalaam , I have made the requested changes by skipping Please let me know if further adjustments are needed or if everything looks good now. Thanks! |
Sounds to me like the right change.
I just kicked off the tests but I am strongly guessing they will fail after your latest change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests fail because they test for the old behaviour before the recent change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disabling logging altogether seems like a very bad idea. How will you diagnose any issues that occur?
if (tracingEnabled) { | ||
new RustSdkCryptoJs.Tracing(RustSdkCryptoJs.LoggerLevel.Debug).turnOn(); | ||
} else { | ||
// Do not initialize tracing when disabled | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem with this is that incorrectly gives the impression that tracing can be enabled or disabled at the level of an individual instance of RustCrypto
.
For example: if I call initRustCrypto({tracingEnabled: true})
, and then later, for a different MatrixClient instance, call initRustCrypto({tracingEnabled: false})
, tracing will be enabled for the second instance despite tracngEnabled
being set to false.
What we really need is to do the work in matrix-sdk-crypto-wasm
to connect the logging from the rust tracing system to logger
, so that it can be correctly enabled or disabled via the js-sdk's logging API.
Checklist
public
/exported
symbols have accurate TSDoc documentation.Fixes
fixes #4177
Description
This PR introduces an optional parameter, tracingEnabled, to the initRustCrypto function, allowing users to enable or disable tracing in the Rust crypto SDK.
Now, the tracing feature can be toggled:
Enabled: tracingEnabled: true - Tracing will capture debug-level logs.
Disabled: tracingEnabled: false - No tracing will occur, leading to cleaner log output and improved performance.
Signed-off-by: Srayash [email protected]