-
Notifications
You must be signed in to change notification settings - Fork 233
Feature/fga/rust sdk tracing #1036
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
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.
I tested it locally and it seems to work well, and the code makes sense to me, although I have a question. I'll review it again once it's ready!
val tracingConfiguration = if (BuildConfig.DEBUG) { | ||
TracingConfiguration( | ||
filterConfiguration = TracingFilterConfigurations.debug, | ||
writesToStdout = false, |
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.
Why is this false
in both cases? I expected it to be true so the lines are displayed in the logcat output and not just written to the log files.
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.
Its always logging to logcat... I asked for a flag for logcat, but rust team removed it...
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.
I guess it's a special case for iOS? I remember they mentioned something about using stderr
and it seems like setting this to true
pipes the output to stderr
.
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.
Maybe ? We don't need it on our side. What do you think about not logging to logcat in release ?
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.
As long as we have those log messages written to our log files I guess it's ok.
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
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.
LGTM!
Kudos, SonarCloud Quality Gate passed! |
Use the new rust api for tracing :
Timber.Tree
delegating tolog_event
TracingConfiguration
so we can directly log to files from the rustTested on a local version of the rust sdk, waiting for matrix-org/matrix-rust-sdk#2384