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

extend log filtering #1725

Merged
merged 2 commits into from
Mar 24, 2023
Merged

extend log filtering #1725

merged 2 commits into from
Mar 24, 2023

Conversation

onur-ozkan
Copy link
Member

@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 changed the title initialize env_logger extend log filtering Mar 18, 2023
@onur-ozkan
Copy link
Member 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

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

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

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

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

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

shamardy
shamardy previously approved these changes Mar 22, 2023
Copy link
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.

🔥

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: ozkanonur <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants