Skip to content

Comments

File Logging#209

Merged
jking-aus merged 41 commits intosigp:unstablefrom
ThreeHrSleep:logging
Apr 29, 2025
Merged

File Logging#209
jking-aus merged 41 commits intosigp:unstablefrom
ThreeHrSleep:logging

Conversation

@ThreeHrSleep
Copy link
Contributor

@ThreeHrSleep ThreeHrSleep commented Mar 25, 2025

Issue Addressed

#208
file logging should work now
still need to work on getting libp2p & discv5 logs though
edit1: (removed the stuff from here,will handle them in separate PR)
edit2: added libp2p & discv5 stuff back in here as the fix was small enough

@ThreeHrSleep ThreeHrSleep marked this pull request as ready for review April 3, 2025 05:59
@ThreeHrSleep ThreeHrSleep added the ready-for-review This PR is ready to be reviewed label Apr 3, 2025
@ThreeHrSleep ThreeHrSleep requested a review from dknopik April 3, 2025 06:08
@dknopik
Copy link
Member

dknopik commented Apr 3, 2025

Few notes from my side:

  • We should use a default log directory if none were set, e.g. /logs
  • I added --debug-level debug but see only INFO for some reason
  • Differing levels for console and file would be nice if possible, so that the user (console) only sees info, but we have debug in the file for troubleshooting. I think it's done this way in LH as well

Copy link
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

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

Just giving this a quick review, but I didn't test anything so was just looking over the code. Hopefully this is helpful

@ThreeHrSleep
Copy link
Contributor Author

ThreeHrSleep commented Apr 7, 2025

Thanks for the review(s)!
i think all the suggestions should be covered now : )

@ThreeHrSleep ThreeHrSleep requested a review from macladson April 7, 2025 05:37
Copy link
Member

@macladson macladson left a comment

Choose a reason for hiding this comment

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

Didn't get a chance to do a very thorough review but just had some more code structure comments for you

@ThreeHrSleep
Copy link
Contributor Author

Thanks again @macladson !! : )
Could you please take one last look to confirm if we're good to go(or if anything needs fixing😅) ?

@dknopik
Copy link
Member

dknopik commented Apr 16, 2025

Tried this out on interop, there seem to be a few issues. Will dig into it more later.

@dknopik
Copy link
Member

dknopik commented Apr 22, 2025

There seems to be a bug as libp2p_gossipsub and discv5 logs show up in the anchor.log file.

EDIT: actually, all three log files seem to contain all logs.

@dknopik
Copy link
Member

dknopik commented Apr 22, 2025

Without this branch, cargo run -- node -t not_existing_dir returns

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 12.92s
     Running `target/debug/anchor node -t not_existing_dir`
2025-04-22T13:18:37.668948Z ERROR anchor: Unable to initialize configuration e="Unable to read \"not_existing_dir/ssv_contract_address.txt\": No such file or directory (os error 2)"

With this branch, it returns

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.62s
     Running `target/debug/anchor node -t not_existing_dir`

so apparently, the error is suppressed.

@ThreeHrSleep
Copy link
Contributor Author

Thanks for catching all the issues ! (& apologies for not testing the changes properly myself)
all of these should fixed now(hopefully),

please lmk if i broke anything else this time : )

@dknopik dknopik mentioned this pull request Apr 28, 2025
@ThreeHrSleep ThreeHrSleep requested a review from dknopik April 28, 2025 15:01
Copy link
Member

@dknopik dknopik left a comment

Choose a reason for hiding this comment

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

small nitpick

Otherwise, LGTM, seems to work well on interop. Can someone else take an additional look before merging?

Co-authored-by: Daniel Knopik <107140945+dknopik@users.noreply.github.com>
@jking-aus jking-aus merged commit 8c6b305 into sigp:unstable Apr 29, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review This PR is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants