Skip to content

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

Merged
merged 10 commits into from
Aug 9, 2023
Merged

Feature/fga/rust sdk tracing #1036

merged 10 commits into from
Aug 9, 2023

Conversation

ganfra
Copy link
Member

@ganfra ganfra commented Aug 8, 2023

Use the new rust api for tracing :

  • Provides a Timber.Tree delegating to log_event
  • Use the new TracingConfiguration so we can directly log to files from the rust
  • Clean files older than 24hours, waiting for a proper rotation from the rust

Tested on a local version of the rust sdk, waiting for matrix-org/matrix-rust-sdk#2384

@ganfra ganfra requested a review from jmartinesp August 8, 2023 09:46
Copy link
Member

@jmartinesp jmartinesp left a 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,
Copy link
Member

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.

Copy link
Member Author

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...

Copy link
Member

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.

Copy link
Member Author

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 ?

Copy link
Member

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.

@ganfra ganfra marked this pull request as ready for review August 9, 2023 09:28
@ganfra ganfra requested a review from a team as a code owner August 9, 2023 09:28
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/bz47d4

Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

LGTM!

@ganfra ganfra enabled auto-merge August 9, 2023 10:04
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@ganfra ganfra added this pull request to the merge queue Aug 9, 2023
Merged via the queue into develop with commit e8d1f21 Aug 9, 2023
@ganfra ganfra deleted the feature/fga/rust_sdk_tracing branch August 9, 2023 10:19
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