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

feat: client debug logging #58

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

TheoTechnicguy
Copy link
Contributor

Hello everyone!

tl;dr: this PR improves logging.

While debugging #35 and #38, I have added some extra logging statements here and there in the application to be able to follow the execution flow. I then set out to add logging a bit everywhere.

My logging rationale is as follows

  • trace: used to trace the code, follow execution flow and find one exact code part.
  • debug: information and output that is diagnostically helpful to understand what is going on.
  • info: general use information
  • warn: information about oddities, but for which recovering is planned.
  • error: operation results that are expected to work but did not, but do not force program termination.
  • fatal: circumstances that force the program to terminate.

Does that sound right? At the moment, I have been using mostly debug, as there is no option for trace. Should that be implemented?
Also, is it okay to give warnings (and up) to the user, or should there be a separate feedback for the user (on stderr)?

On the subject of the code, I have just added logging statements in my style where I think them useful. Am I too aggressive? Is my (variable) logging convention acceptable for the project?

Overall, please share what you think about the changes and what else I should incorporate ^^.

@TheoTechnicguy TheoTechnicguy changed the title draft: feat: debug logging draft: feat: client debug logging Dec 29, 2023
@TheoTechnicguy TheoTechnicguy marked this pull request as ready for review December 29, 2023 14:50
@TheoTechnicguy TheoTechnicguy changed the title draft: feat: client debug logging feat: client debug logging Dec 29, 2023
@francoismichel
Copy link
Owner

What is the status of this PR ? Do you want me to drop a review anytime soon ?

@TheoTechnicguy
Copy link
Contributor Author

Hi! Sorry for the lack of communication. I have been kept busy with exam preparations.

In my opinion, the request is ready to be reviewed (I'll do my best to keep up with resoling conflicts and change requests). I can't seem to request reviews, though…

Could you confirm or correct the proposed debugging level convention, from above? I'd really help me to make sure that I use the appropriate level, as (atm) everything is debug and I think I went a bit too much into detail.

@francoismichel
Copy link
Owner

Allright, focus on your exams, I'll do the rebasing and make a review

@francoismichel
Copy link
Owner

Okay so I did not see that the branches still differ quite a lot. We can easily wait until the end of your exams and then look into it into more details. The changes you added are valuable, but the ssh3 upstream evolved quite a lot since its initial release (+ files have been renamed), so we should probably adapt these changes to the current state once you have time. :-)

@TheoTechnicguy
Copy link
Contributor Author

Apologies for not keeping up with the main branch. I managed to merge the main branch. Checking the changes over in the files changed tab shows that there are only additions. After skimming over them, I am confident that I did not create side effects. 😄
The fact that tests pass also reassures me ^^

I hope everything is ready to be reviewed — let me know otherwise.

@TheoTechnicguy
Copy link
Contributor Author

Hi @francoismichel !

I'm all done with exams now and am available again to contribute to the project. I just merged it with the latest reference from the main branch, so it should be up-to-date. Could you review this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants