Tracing logs by cryptoAtwill · Pull Request #432 · consensus-shipyard/fendermint

cryptoAtwill · 2023-11-15T03:36:36Z

Add more logging outputs options so that we still have default info log to console, but also have debug logs with rotation written to file.

…into skip-null-round-retry

…into tracing-logs

adlrocha · 2023-11-15T06:47:21Z

docker/runner.Dockerfile

+
+RUN useradd ipc && chown -R ipc $FM_HOME_DIR
+
+USER ipc


Why do we need an ipc user instead of root here? Also, I don't think it is the case but I am wondering if this may break the execution of the fendermint container considering the directory structure that we use. Have you tried it?

I never quite understood how to use users inside containers properly. I made images where I used add-user to create users with specific IDs and groups, but unless those IDs aligned with what exists on the host, I still had to end up setting the -u option to make sure I can remove files created into mounted volumes without sudo. Since this is a container, I never quite got why we shouldn't use root 🤷 I'm sure I'm just ignorant, but if it works...

I removed as it's not needed anymore. But I think one day we should not use root to execute docker

fendermint/vm/topdown/src/sync.rs

adlrocha · 2023-11-15T06:54:09Z

fendermint/app/src/main.rs

-
-        tracing::subscriber::set_global_default(subscriber)
-            .expect("setting default subscriber failed");
+        create_log(level, &opts.home_dir).expect("cannot create logging");


I think we should add a setting to configure the output path, and if we want the log to be output on std err/std output, or to a file.

I think it's right to do both. I had a similar thing in my Scala programs except I was able to configure them through XML, so we can enable the debug level on a package-by-package basis (third parties as well) without recompiling.

fendermint/app/src/main.rs

adlrocha · 2023-11-15T06:55:01Z

fendermint/app/src/main.rs

+
+    // debug level logger
+    let topdown_logger = config::Logger {
+        level: config::LevelFilter::DEBUG,


Should we make this configurable too? Now we want this in debug mode for debugging purpose, but we may want to keep the same level for all in the future.

I would say we just keep this top down logs just in case for something unexpected. The logs will be 50mb in total, it's not taking a lot of space. But we always have some logs to refer back to.

fendermint/app/options/src/lib.rs

fendermint/app/src/main.rs

fendermint/vm/interpreter/src/chain.rs

fendermint/vm/topdown/src/finality/null.rs

fendermint/vm/topdown/src/sync.rs

Co-authored-by: Akosh Farkash <[email protected]>

fendermint/app/src/main.rs

aakoshh · 2023-11-15T12:15:53Z

fendermint/app/src/main.rs

+        config
+            .appenders
+            .insert(AppenderId::from("topdown"), topdown_appender);


Why do we need to insert the logger into the config, when it's already set in the topdown_logger?

I havent looked very deep into trace4rs, my guess is when an event is emitted, it checks if there are any Target watching this event, then target has a logger, and logger has an appender.

fendermint/app/src/main.rs

* look ahead limit use finalized height * rename variable * more comments * more logs

…dermint into tracing-logs

fendermint/vm/topdown/src/sync.rs

Co-authored-by: Akosh Farkash <[email protected]>

…dermint into tracing-logs

aakoshh · 2023-11-15T13:17:45Z

Cargo.toml

 tracing-subscriber = "0.3"
 url = "2.4.1"
 zeroize = "1.6"
+trace4rs = "0.5.1"


Do you know if this is available on Github somewhere? I can't find a link at https://crates.io/crates/trace4rs

https://github.com/imperva/trace4rs

aakoshh · 2023-11-15T13:19:40Z

fendermint/app/src/main.rs

+        config
+            .loggers
+            .insert(Target::from("fendermint_app"), debug_logger.clone());
+        config
+            .loggers
+            .insert(Target::from("fendermint_vm_interpreter"), debug_logger);


https://docs.rs/trace4rs/0.5.1/src/trace4rs/handle/loggers.rs.html#91 suggests that a simple Target::from("fendermint") should work.

funny thing is, with this, all logs are directly to debug and console is empty

Maybe because it's set as a "default" which could mean "if nothing else matched"? Perhaps insert it explicitly with the "" target?

Yes 👉 https://docs.rs/trace4rs/0.5.1/src/trace4rs/handle/layers.rs.html#139-149

aakoshh · 2023-11-15T13:21:26Z

fendermint/vm/topdown/src/sync.rs

-                "parent block hash at {height} is {:02x?} diff than previous hash: {previous_hash:02x?}",
-                block_hash_res.parent_block_hash
-            );
+            heigit = hex::encode(&block_hash_res.parent_block_hash),


Suggested change

heigit = hex::encode(&block_hash_res.parent_block_hash),

height,

parent_hash = hex::encode(&block_hash_res.parent_block_hash),

fendermint/app/src/main.rs

Co-authored-by: Akosh Farkash <[email protected]>

…dermint into tracing-logs

aakoshh · 2023-11-15T13:36:43Z

fendermint/app/src/main.rs

+            .insert(Target::from("fendermint"), debug_logger);
+        config
+            .appenders
+            .insert(AppenderId::from("debug"), debug_appender);


Yeah so why don't we insert a clone of the default console logger with an Target::from("") and the INFO level?

yeah, but it seems to me this is a bug or sth, we should not be able to do that though. But I added this.

What's a bug? Why shouldn't we be able to set a logger that captures everything and logs to the console if we want to?

I mean we need to do this:

config .loggers .insert(Target::from("fendermint"), debug_logger); config.loggers.insert(Target::from("fendermint"), default);

but ideally we should not need config.loggers.insert(Target::from("fendermint"), default); as we already set default.

Ah, okay, so if fendermint* will go to both debug files and console, and everything else will also go to the console because it matches all namespaces, if their log level is at least the selected one 👍

aakoshh

Thank you for all the changes!

cryptoAtwill added 8 commits November 14, 2023 19:21

skip null round retry

a5117db

rearrange logging

43bf8fe

Merge branch 'main' of protocol-github:consensus-shipyard/fendermint …

d485555

…into skip-null-round-retry

specify import

ed5ab9d

better null error handling

5796d5c

format code

961b6be

add tracing logs

a205b2e

expand tilda

b83dd8e

cryptoAtwill requested review from aakoshh and adlrocha November 15, 2023 03:36

cryptoAtwill added 6 commits November 15, 2023 11:41

prettier logging

bba832b

prettier logs

3d11185

update logging

5a5c134

prettier log

d6b0f0d

Merge branch 'main' of protocol-github:consensus-shipyard/fendermint …

9a585f1

…into tracing-logs

use ipc user instead of root

e4873a6

adlrocha reviewed Nov 15, 2023

View reviewed changes

specify log path

884a5a2

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/app/options/src/lib.rs Outdated Show resolved Hide resolved

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/app/src/main.rs Outdated Show resolved Hide resolved

remove unused logs

c83dde3

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/app/src/main.rs Outdated Show resolved Hide resolved

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/vm/interpreter/src/chain.rs Show resolved Hide resolved

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/vm/interpreter/src/chain.rs Show resolved Hide resolved

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/vm/topdown/src/finality/null.rs Outdated Show resolved Hide resolved

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/vm/topdown/src/sync.rs Show resolved Hide resolved

cryptoAtwill and others added 4 commits November 15, 2023 16:48

Update fendermint/vm/topdown/src/finality/null.rs

87445a6

Co-authored-by: Akosh Farkash <[email protected]>

Update fendermint/vm/interpreter/src/chain.rs

2fa6c84

Co-authored-by: Akosh Farkash <[email protected]>

Update fendermint/vm/topdown/src/sync.rs

6bb0bf0

Co-authored-by: Akosh Farkash <[email protected]>

structured logging and log dir optional

ae9aed1

cryptoAtwill requested review from aakoshh and adlrocha November 15, 2023 11:02

adlrocha approved these changes Nov 15, 2023

View reviewed changes

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/app/src/main.rs Outdated Show resolved Hide resolved

aakoshh reviewed Nov 15, 2023

View reviewed changes

cryptoAtwill added 2 commits November 15, 2023 20:19

add debug logger

8e73396

add debug logs

5d3113f

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/app/src/main.rs Outdated Show resolved Hide resolved

cryptoAtwill and others added 4 commits November 15, 2023 20:48

log fendermint only

0e1bcc3

Look ahead limit use finalized height (#433)

f79f7f3

* look ahead limit use finalized height * rename variable * more comments * more logs

comments

7a9e257

Merge branch 'tracing-logs' of protocol-github:consensus-shipyard/fen…

6810ed9

…dermint into tracing-logs

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/vm/topdown/src/sync.rs Outdated Show resolved Hide resolved

cryptoAtwill and others added 3 commits November 15, 2023 21:02

Update fendermint/vm/topdown/src/sync.rs

dde324d

Co-authored-by: Akosh Farkash <[email protected]>

rename makes everything clearer

636bdc5

Merge branch 'tracing-logs' of protocol-github:consensus-shipyard/fen…

d2d2fa8

…dermint into tracing-logs

aakoshh reviewed Nov 15, 2023

View reviewed changes

fendermint/app/src/main.rs Outdated Show resolved Hide resolved

cryptoAtwill and others added 3 commits November 15, 2023 21:27

Update fendermint/app/src/main.rs

0ba8f9c

Co-authored-by: Akosh Farkash <[email protected]>

clean logs

46022c9

Merge branch 'tracing-logs' of protocol-github:consensus-shipyard/fen…

2e374f0

…dermint into tracing-logs

aakoshh reviewed Nov 15, 2023

View reviewed changes

log to console

bbbb8a5

aakoshh approved these changes Nov 15, 2023

View reviewed changes

update docker commands

6d94af3

cryptoAtwill merged commit ded95dd into main Nov 15, 2023

cryptoAtwill deleted the tracing-logs branch November 15, 2023 14:17


		RUN useradd ipc && chown -R ipc $FM_HOME_DIR

		USER ipc No newline at end of file

	heigit = hex::encode(&block_hash_res.parent_block_hash),
	height,
	parent_hash = hex::encode(&block_hash_res.parent_block_hash),

Conversation

cryptoAtwill commented Nov 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aakoshh Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aakoshh Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aakoshh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aakoshh Nov 15, 2023 •

edited

Loading

aakoshh Nov 15, 2023 •

edited

Loading