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

Change the log library from fimber to logging #657

Merged
merged 4 commits into from
Oct 9, 2023

Conversation

ademar111190
Copy link
Collaborator

@ademar111190 ademar111190 commented Oct 5, 2023

Reasons to change from Fimber to Logging:

Drawbacks

  • We lost the SizeRollingFileTree
    • I changed the logic to keep the last ten sessions as it seems a reasonable heuristic to me; Let me know your thoughts
  • LogCat integration as mentioned earlier

How to test

  • Everyone:
    • Goes to Developers -> three dots menu -> share logs
      • A zip file with the last 10 sessions (plus the current sessions) should be shared
  • For devs:
    • Make sure the log shows on your console (debug flavors only)

Please also review the SDK side of the changes

breez/breez-sdk-greenlight#505

Fixes #588
Fixes #587

Copy link
Contributor

@ubbabeck ubbabeck left a comment

Choose a reason for hiding this comment

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

code looks good and tested

works with logcat and vscode dev console and easy to filter.

exporting the logs also works.

@ubbabeck ubbabeck requested a review from roeierez October 6, 2023 05:13
@ademar111190
Copy link
Collaborator Author

  • Just an addendum for clarity, we lost Logcat integrations but we still have basic functionalities working such as the output and filter messages by regex and by package

@ademar111190 ademar111190 force-pushed the aa/change-log-library branch from 43f8ca8 to 1b763ac Compare October 6, 2023 11:57
Copy link
Collaborator

@erdemyerebasmaz erdemyerebasmaz left a comment

Choose a reason for hiding this comment

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

LGTM!

Please re-trigger CI after breez-sdk changes are in place before merging, ty.

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Looks good!
One think I am missing is the sdk logs.
Now that the sdk logs exposes breezLogStream shouldn't we use it here and log the records?

@ademar111190
Copy link
Collaborator Author

Looks good! One think I am missing is the sdk logs. Now that the sdk logs exposes breezLogStream shouldn't we use it here and log the records?

Yes; I'm doing it here:

@ademar111190 ademar111190 force-pushed the aa/change-log-library branch from b4336d9 to b170533 Compare October 9, 2023 12:07
@ademar111190 ademar111190 merged commit 5823fe0 into main Oct 9, 2023
1 check passed
@ademar111190 ademar111190 deleted the aa/change-log-library branch October 9, 2023 12:22
@ademar111190 ademar111190 mentioned this pull request Dec 8, 2023
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.

Some log lines seem to be truncated logs should be in UTC time
4 participants