Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: warn log on conn lost #170

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

dxps
Copy link
Contributor

@dxps dxps commented Jul 31, 2022

First of all thank you so much for doing and sharing this MQTT 5 Server. 🙏
At least in my initial searches, this was the only result of a Go(lang) based MQTT 5 Server.
And I'm planning to contribute to it in my following weeks and months.


I know it might be just a picky thing and maybe I'm used to see stack traces when something really bad or unknown happened (in which case a stack trace that should bring enough context details).

When I tested initially with a paho.golag client (which works just fine, btw, at least for now), in case of an authentication failure (either user doesn't exist or client provide wrong user credentials), the server (gmqttd) logs the error but also the stack trace. And that's defined on purpose in config/config.go:237) by:
zaplog := zap.New(core, zap.AddStacktrace(zap.ErrorLevel), zap.AddCaller())

Since to me it looks like something really bad happened.
image

And that's why I'm proposing this small change for now: a connection lost should be logged as warning.
Later, I would include one additional logging config item to specify at which level you'd like to have the stack trace included.

@DrmagicE
Copy link
Owner

DrmagicE commented Aug 1, 2022

Thanks for the contribution. I agree that warning would be more appropriate in this situation.

@DrmagicE DrmagicE merged commit fc1d12c into DrmagicE:master Aug 1, 2022
@dxps dxps deleted the warn_log_for_conn_lost branch August 5, 2022 09:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants