Skip to content

extend log filtering#1725

Merged
onur-ozkan merged 2 commits intodevfrom
enable-log-management
Mar 24, 2023
Merged

extend log filtering#1725
onur-ozkan merged 2 commits intodevfrom
enable-log-management

Conversation

@onur-ozkan
Copy link
Copy Markdown

@onur-ozkan onur-ozkan commented Mar 17, 2023

The custom log filtering implementation limits users for no reason. We already have env_logger in our dependency tree. With this PR, we will be able to change log level for any module from RUST_LOG.

e.g: export RUST_LOG="debug,atomicdex_gossipsub::behaviour=off,crypto::crypto_ctx=warn"

Global default filter is "info". If you want to debug specific module for example atomicdex_gossipsub::behaviour and nothing else, you can do export RUST_LOG="info,atomicdex_gossipsub::behaviour=debug" and that will keep default level as info with printing debug logs from atomicdex_gossipsub::behaviour.

cc @Alrighttt

@onur-ozkan onur-ozkan requested review from ca333 and shamardy March 17, 2023 18:45
@onur-ozkan onur-ozkan changed the title initialize env_logger extend log filtering Mar 18, 2023
@onur-ozkan onur-ozkan force-pushed the enable-log-management branch from f940fb4 to ed5c72b Compare March 18, 2023 10:13
@onur-ozkan
Copy link
Copy Markdown
Author

I am not sure if wasm side will be as stable as before. @yurii-khi could be nice if you can test/check this for wasm

@yurii-khi
Copy link
Copy Markdown

I am not sure if wasm side will be as stable as before. @yurii-khi could be nice if you can test/check this for wasm

Hey! Do you have any specific test-cases in mind, or just general smoke testing?

@onur-ozkan
Copy link
Copy Markdown
Author

onur-ozkan commented Mar 20, 2023

I am not sure if wasm side will be as stable as before. @yurii-khi could be nice if you can test/check this for wasm

Hey! Do you have any specific test-cases in mind, or just general smoke testing?

Just log specific testing for wasm. I want to make sure I didn't break any of the log functionality for wasm

@yurii-khi
Copy link
Copy Markdown

I am not sure if wasm side will be as stable as before. @yurii-khi could be nice if you can test/check this for wasm

Hey! Do you have any specific test-cases in mind, or just general smoke testing?

Just log specific testing for wasm. I want to make sure I didn't break any of the log functionality for wasm

Right now it breaks our logger, most probably because of missing LogLevel, @anarkiVan could you please take a closer look?

@onur-ozkan
Copy link
Copy Markdown
Author

onur-ozkan commented Mar 20, 2023

Because LogLevel is excluded from mm2. So it shouldn't be provided.

But I am also not sure if env_logger will work for wasm. Let me keep it simple and rollback LogLevel for wasm target. Not worth making breaking change there.

Can you provide me very simple js code for testing this logging thing? @yurii-khi

@yurii-khi
Copy link
Copy Markdown

I think best way would be to use exact js file we're using on webdex side:
https://github.com/KomodoPlatform/WebDEX/blob/dev/web/src/index.js

But I'd recommend to install flutter and run app locally, it's very simple and will provide reliable results.
https://github.com/KomodoPlatform/WebDEX/blob/add/docs/docs/project_setup.md
https://github.com/KomodoPlatform/WebDEX/blob/add/docs/docs/update_wasm_module.md

@onur-ozkan
Copy link
Copy Markdown
Author

We removed the custom LogLevel implementation completely. Since it increases the complexicty for literally "no reason". It doesn't provide any flexibility or feature.

What happens if I remove it for wasm as well? I removed them in this PR and need a feedback from gui team. Does LogLevel thing is really needed for gui side?

For testing it, https://github.com/KomodoPlatform/WebDEX/blob/b8f05db785783889e659ea67f36dd7313f76d55c/web/src/index.js#L37 this line needs to be removed with LogLevel all together because they don't exists anymore with this PR.

cc @yurii-khi @anarkiVan

@yurii-khi
Copy link
Copy Markdown

We removed the custom LogLevel implementation completely. Since it increases the complexicty for literally "no reason". It doesn't provide any flexibility or feature.

What happens if I remove it for wasm as well? I removed them in this PR and need a feedback from gui team. Does LogLevel thing is really needed for gui side?

For testing it, https://github.com/KomodoPlatform/WebDEX/blob/b8f05db785783889e659ea67f36dd7313f76d55c/web/src/index.js#L37 this line needs to be removed with LogLevel all together because they don't exists anymore with this PR.

cc @yurii-khi @anarkiVan

I think we can get rid of it on our side as well, just need to wait until changes propagate to dev branch.

@onur-ozkan
Copy link
Copy Markdown
Author

It can be nice if you can test this branch on gui side for wasm, to see if there is nothing wrong after removing LogLevel

@yurii-khi
Copy link
Copy Markdown

yurii-khi commented Mar 22, 2023

It can be nice if you can test this branch on gui side for wasm, to see if there is nothing wrong after removing LogLevel

Tested ed5c72b3b build with changes on our side, mentioned above (deleted LogLevel completely), and it works fine, was able to run the app and swap some RICK->MORTY. I have a few questions:

  1. Does it pass all the mm2 logs to handle_log atm? It looks like amount of log output has been reduced significantly.
  2. Is there a way to separate errors from other messages, so we can treat them differently?

@onur-ozkan
Copy link
Copy Markdown
Author

onur-ozkan commented Mar 22, 2023

Is there a way to separate errors from other messages, so we can treat them differently?

No, not if we remove LogLevel completely. Seems like I will rollback removing LogLevel only for wasm target.

It turns out doing that will likely increase compexicty to higher levels. This requires much more refactoring than I expected. I will only keep enabling env_logger for this particular PR since it's needed for our team. Log refactoring will be later in different PR.

@onur-ozkan onur-ozkan force-pushed the enable-log-management branch from ed5c72b to 1bfd6b5 Compare March 22, 2023 15:49
shamardy
shamardy previously approved these changes Mar 22, 2023
Copy link
Copy Markdown
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

🔥

Comment thread CHANGELOG.md Outdated
Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan merged commit 5f4a8bb into dev Mar 24, 2023
@onur-ozkan onur-ozkan deleted the enable-log-management branch March 24, 2023 11:17
@ca333 ca333 mentioned this pull request Mar 27, 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.

3 participants