Improved Error Handling#2801
Conversation
|
Thank you for the pr. Unfortunately I see two problems with it:
|
|
I had no idea. OK I will address those when I get a sec and re-open; wasn't sure of how the workflow goes so I went with what I had. So, a question: do I at least add the entry in |
You can leave it in for testing but before merging to master it needs to be removed. |
|
So then how do we know what string to translate to other languages? Can I go into weblate and add the english translation myself? Apologies for not understanding here; I've never worked with a project including translations before. |
| _ => ex.Message | ||
| }; | ||
|
|
||
| Log.ErrorSensitive("Unable to contact remote server: {ip}:{port}. {reason}", serverIp, serverPort, ex.Message); |
There was a problem hiding this comment.
This is now English, ex.Message is the English error before translation.
| "Nitrox_TeleportToQuestion": "Teleport to {PLAYER}?", | ||
| "Nitrox_ThankForPlaying": "Thanks for using Nitrox!", | ||
| "Nitrox_UnableToConnect": "Unable to contact the remote server at:", | ||
| "Nitrox_NoConnection": "No network connection available.", |
There was a problem hiding this comment.
This can be changed before merge. For now, I will keep it so we have reference strings.
OK, I have made both changes you requested. 👍 |
lammmab
changed the title
| if (endpoint == null) | ||
| { | ||
| Log.InGame($"{Language.main.Get("Nitrox_UnableToConnect")}: {serverIp}:{serverPort}"); | ||
| Log.InGame($"{Language.main.Get("Nitrox_ServerNotFound")}: {serverIp}:{serverPort}"); |
There was a problem hiding this comment.
What is the difference in the Nitrox_UnableToConnect message vs the new Nitrox_ServerNotFound? Isn't it technically the same outcome?
|
|
||
| IsConnected = false; | ||
| Log.Info("Disconnected from server"); | ||
| Log.Error($"Disconnected from server. Reason: {disconnectInfo.Reason}, SocketError: {disconnectInfo.SocketErrorCode}"); |
There was a problem hiding this comment.
Should we log an error if we disconnected gracefully?
| private DisconnectInfo? lastDisconnectInfo; | ||
| public DisconnectInfo? ConnectFailReason => lastDisconnectInfo; |
There was a problem hiding this comment.
Minor: Would maybe compact it into a property. Right now the both names convey different meaning of the "same" variable.
| using System.Net.NetworkInformation; | ||
| using System.Linq; |
| && n.NetworkInterfaceType != NetworkInterfaceType.Loopback | ||
| && n.NetworkInterfaceType != NetworkInterfaceType.Tunnel)) | ||
| { | ||
| throw new ClientConnectionFailedException(Language.main.Get("Nitrox_NoConnection")); |
There was a problem hiding this comment.
In every case in this file where we translate the message in the constructor we will be logging it translated in the logs, or am I wrong?
2 participants
Linked Issues / Context
I found no relevant issues. Feel free to tag them.
Description of Change
This PR aims to improve error output for common server errors that users may face. It took threading through a few files, and the most common error will be "Nitrox_NoResponse", but I feel it is still a great improvement over the generic "Unable to contact remote server".
Validation
PR Checklist
mainat the time of openingNotes
Some errors I could not hit testing locally. Someone with a more advanced setup will want to test all of the error cases and see if they get improved debug output.
I am on Arch Linux.