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

stream stdout/err to a file #1751

Merged
merged 7 commits into from
Apr 11, 2023
Merged

stream stdout/err to a file #1751

merged 7 commits into from
Apr 11, 2023

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Apr 10, 2023

refactors native_log and implements stdout/err streaming to persistent file without dependencies

removed dependencies from dependency tree:

  • arc-swap@0.4.8
  • log-mdc@0.1.0
  • thread-id@3.3.0
  • log4rs@1.0.0
  • env_logger@0.7.1
  • humantime@1.3.0

ref #1749 (comment)

Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
Signed-off-by: ozkanonur <work@onurozkan.dev>
@cipig
Copy link
Member

cipig commented Apr 10, 2023

CLI works fine, as before the change
on ADEX Desktop, the logfile works too
some entries are duplicated, shown in log and on console, like this ones during a swap

10 15:32:47, mm2_main::mm2::lp_swap::taker_swap:2425] WARN taker_swap:2478] utxo_common:3362] utxo_common:936] Not enough RVN for swap: available 0, required at least 0.00001, locked by swaps None
10 15:32:48, mm2_main::mm2::lp_swap::taker_swap:1327] INFO Got maker payment 5f175b31a67207fed87a043bacf3f7c8253f3a1147a164882e1165618898e34a
10 15:32:48, mm2_main::mm2::lp_swap::taker_swap:1340] INFO Before wait confirm

is that intentionally?
i am running an ADEX Desktop debug build, will test with a release build, idk if it makes a difference

@cipig
Copy link
Member

cipig commented Apr 10, 2023

it's the same on the release build of ADEX Desktop, the entries are written to the logfile, but also showed on console

@onur-ozkan
Copy link
Member Author

Stdout/err is directly streamed into the log file, whatever you see on the terminal is also in the log file. Specially for users who's deploying lots of mm2 on server, you can't track all the terminals up-to-bottom. So you will be able to search in files which I believe much easier.

Was this different before? Didn't we already write the terminal logs into the file before?

@cipig
Copy link
Member

cipig commented Apr 10, 2023

ADEX Desktop handles his own logs like this atm:

  • the debug build shows it on console and writes it to file
  • the release build only writes it to file

that is for his own logs, not from mm2

the stuff from mm2 is shown on console and written to the file, on both debug and release builds of Desktop
Desktop starts mm2 with the logfile param set for mm2

on pure CLI, i don't use the logfile param from mm2, i simply redirect the console output to file... i have done it like this before and haven't changed anything... this works like it was before, no change, all good

@cipig
Copy link
Member

cipig commented Apr 10, 2023

how is it meant to be? when i set logfile in mm2... should the output be duplicated on console and log file?... or should it be only in log file? atm it's on both

if it's meant to be on both, then it's fine and works as designed

@onur-ozkan
Copy link
Member Author

how is it meant to be? when i set logfile in mm2... should the output be duplicated on console and log file?... or should it be only in log file? atm it's on both

if it's meant to be on both, then it's fine and works as designed

When you set the MM_LOG, the stdout and stderr will be written in the file you specify in MM_LOG. If you want to make your terminal clean/empty and write the logs only on the file, I can add a parameter to mm conf, like silent_console, which will prevent writing logs to the terminal. Do you want me to do that?

@onur-ozkan
Copy link
Member Author

The logging implementation on mm2 is quite complicated and chaotic I would say. And I am trying to adapt it to the common/best practices. Ofc without breaking the mm2 clients. So if you want to keep the terminal clean and write logs to the log file, I will have to add another parameter to the mm2 conf(the file you give to MM_CONF_PATH).

Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

onur-ozkan commented Apr 10, 2023

With 861ce8e you will be able to keep the console empty and write the logs to the file. If you don't enable file logging and make silent_console true, there will be no logging anywhere. silent_console is false as a default, which can be enabled anytime from configuration file.

Let me know if this does what desktop client needs @cipig

@cipig
Copy link
Member

cipig commented Apr 10, 2023

Let me know if this does what desktop client needs

I actually don't know what Desktop app wants. But if there is a choice it's perfect, they can decide and configure accordingly.
I saw that console output was completely removed from Desktop release builds, so i thought that having suddenly mm2 output on console would be counterproductive.

cipig
cipig previously approved these changes Apr 10, 2023
@onur-ozkan
Copy link
Member Author

With 861ce8e you will be able to keep the console empty and write the logs to the file. If you don't enable file logging and make silent_console true, there will be no logging anywhere. silent_console is false as a default, which can be enabled anytime from configuration file.

I think silent_console should be documented once this PR is merged

cc @yurii-khi @smk762

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.

Thanks a lot for the refactor! only one small comment.

Cargo.lock Outdated Show resolved Hide resolved
Signed-off-by: ozkanonur <work@onurozkan.dev>
@shamardy shamardy merged commit 54fd58a into dev Apr 11, 2023
@shamardy shamardy deleted the log-fixes-enhancements branch April 11, 2023 13:03
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