Skip to content

File Logging #209

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 31 commits into
base: unstable
Choose a base branch
from
Open

File Logging #209

wants to merge 31 commits into from

Conversation

ThreeHrSleep
Copy link
Member

@ThreeHrSleep ThreeHrSleep commented Mar 25, 2025 •
edited
Loading

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

macladson
macladson reviewed Apr 3, 2025
View reviewed changes
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
Member Author

ThreeHrSleep commented Apr 7, 2025 •
edited
Loading

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
macladson
macladson reviewed Apr 7, 2025
View reviewed changes
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.

I still have some more things to review but here's some things I noticed for now

@ThreeHrSleep ThreeHrSleep force-pushed the logging branch 2 times, most recently from e3b84e7 to 9530d77 Compare April 9, 2025 03:26
macladson
macladson reviewed Apr 12, 2025
View reviewed changes
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
Member Author

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

@ThreeHrSleep ThreeHrSleep requested a review from macladson April 16, 2025 03:35
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dknopik dknopik Awaiting requested review from dknopik

@macladson macladson Awaiting requested review from macladson

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
do-not-merge ready-for-review This PR is ready to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ThreeHrSleep @dknopik @macladson