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

Add logging #84

Merged
merged 2 commits into from
May 19, 2022
Merged

Add logging #84

merged 2 commits into from
May 19, 2022

Conversation

lukebakken
Copy link
Contributor

Fixes #81

@lukebakken lukebakken added this to the 1.4.0 milestone May 17, 2022
@lukebakken lukebakken self-assigned this May 17, 2022
@lukebakken lukebakken requested review from ansd and Gsantomaggio May 17, 2022 18:32
@lukebakken lukebakken marked this pull request as ready for review May 17, 2022 18:32
@lukebakken
Copy link
Contributor Author

cc @fho

@lukebakken
Copy link
Contributor Author

I decided to go with the simplest solution and just use the log standard package, without an intermediate interface.

@fho
Copy link
Contributor

fho commented May 18, 2022

Why is logging the errors the right approach to handle them?

If one of those operations fail and cause e.g that a connection becomes unusable this must be either smoothly handled by the package or the error must be forwarded to the user, so the user can react and close the connection.
If a returned error does not cause a problem because e.g. a channel is being closed and the close message can not be send because the connection broke it should be fine to ignore them silently.

log.go Outdated Show resolved Hide resolved
log.go Outdated Show resolved Hide resolved
log.go Outdated Show resolved Hide resolved
log.go Outdated Show resolved Hide resolved
channel.go Outdated Show resolved Hide resolved
ansd
ansd previously requested changes May 18, 2022
Copy link
Member

@ansd ansd left a comment

Choose a reason for hiding this comment

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

I second most (if not all) of the review of @fho

@lukebakken
Copy link
Contributor Author

Why is logging the errors the right approach to handle them?

I didn't want to change the API of the project at this time. But, maybe I shouldn't worry so much about it.

Thanks @fho and @ansd. I suspect I'll be able to remove the need for logging most or all of these errors by changing the API to ensure errors are returned when appropriate.

@lukebakken lukebakken force-pushed the lukebakken/gh-81 branch 2 times, most recently from 973df1e to 4143243 Compare May 18, 2022 15:46
@lukebakken lukebakken requested a review from ansd May 18, 2022 15:47
@lukebakken
Copy link
Contributor Author

@fho @ansd OK it turns out that the cases where we were ignoring errors they can in fact be ignored. However, I still would like the ability to turn on logging just in case someone is having a weird issue and logging these places could assist.

Plus, this gives us a starting place should we wish to add very detailed debug logging at some point.

By default logging is not enabled, though it can be via EnableLogger or SetLogger

Correctly exit test on errors

Get rid of a couple more panics

Replace nolint / TODO with logging

Use single Logger

Add more informative log messages
@lukebakken lukebakken dismissed ansd’s stale review May 19, 2022 14:36

Requested changes have been addressed.

Copy link
Contributor

@fho fho left a comment

Choose a reason for hiding this comment

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

looks great, thank you!

log.go Outdated Show resolved Hide resolved
@lukebakken lukebakken merged commit b068367 into main May 19, 2022
@lukebakken lukebakken deleted the lukebakken/gh-81 branch May 19, 2022 16:14
@lukebakken
Copy link
Contributor Author

Thanks @fho we really appreciate the time you take with this project.

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.

Provide a logging interface
3 participants