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

Extension logging #853

Merged
merged 14 commits into from
Mar 9, 2022
Merged

Extension logging #853

merged 14 commits into from
Mar 9, 2022

Conversation

wirednkod
Copy link
Contributor

This PR covers all-but-one (the last) bullets of Pierre's comment in issue #429 .

Some details

Don't output the logs in the console, but instead keep the logs in some buffer. Only the last ~500 or so lines should to be kept, otherwise our memory usage would continue increasing forever.

I left the info logs to still appear in the console. 1000 lines are kept in memory and shown in the extension's Option page.

Add a semi-hidden way somewhere in the extension that can display these logs. The average Joe doesn't need to know that these logs exist, it's more for developers to be able to understand problems.

The semi-hidden way for now is another tab in the Options page. This will be adjusted in the next UI upgrade from @Goranch3

(maybe) If the ~500 log lines that we keep contain a warning or error, there should be some indication. Again, the average Joe doesn't need to see this, but for developers it's useful to be able to easily see if a warning or error has happened. Warnings or errors are generally bugs in smoldot that should be fixed, but don't necessarily prevent applications from working normally.

2 placeholders are keeping separate from rotating debug logs, the errors and warnings. Even though only 1000 latest lines are are kept from debug logs and then older ones are dissappearing, warning and errors are kept.

Last bullet of Pierre's comment will be addressed in different PR.

A screenshot on the UI perspective can be seen below:
image

@wirednkod wirednkod requested review from tomaka and josepot March 8, 2022 21:24
@tomaka
Copy link
Contributor

tomaka commented Mar 9, 2022

Logs only make sense if they're in the correct context.
Please don't split debug, info, warn and error logs in different windows, they should all keep the order they came in

@wirednkod
Copy link
Contributor Author

wirednkod commented Mar 9, 2022

Logs only make sense if they're in the correct context.
Please don't split debug, info, warn and error logs in different windows, they should all keep the order they came in

In debug list all errors are kept there (i should probably alter the name from "debug" to "all logs" or something). The errors and warns are kept in separate lists in addition, in order to be able to identify them faster. In addition errors and warns do not disappear once the 1000 lines pass. This way even if not identified when it happened, dev can know that something happened, see the warn/error log even out of context at a later time. Initial idea was just a counter, but then i thought that once error rotates away:
a) its not easy to track that it rotated away, so that counter will be reduced when it does
B) dev will not know that something happened if does not see it (even after rotation) in the logs

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Mostly good!

projects/extension/src/background/index.ts Outdated Show resolved Hide resolved
projects/extension/src/background/index.ts Outdated Show resolved Hide resolved
projects/extension/src/background/index.ts Outdated Show resolved Hide resolved
@wirednkod wirednkod requested a review from tomaka March 9, 2022 09:10
@wirednkod wirednkod requested a review from tomaka March 9, 2022 11:17
wirednkod and others added 3 commits March 9, 2022 14:48
Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
@wirednkod wirednkod requested a review from tomaka March 9, 2022 15:57
projects/extension/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
@wirednkod wirednkod merged commit 95edfee into paritytech:main Mar 9, 2022
@wirednkod wirednkod deleted the nik-keep-logs-extension branch June 17, 2022 07:42
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