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

Improve error messages on failures #810

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dagwieers
Copy link
Collaborator

@dagwieers dagwieers commented Aug 31, 2020

I started injecting network failures and this makes it handle some of them better.

This relates to #385

Before::
screenshot000

After:
screenshot001

@dagwieers dagwieers added the enhancement New feature or request label Aug 31, 2020
@dagwieers
Copy link
Collaborator Author

My testing currently is limited to the following:

  • Reject DNS access to outside world (Connection refused)
  • Block DNS access to outside world (Timeout)
  • Reject HTTP(S) access to outside world (Connection refused)
  • Block HTTP(S) access to outside world (Timeout)
  • Null-route destination (No route to host)
  • Kill firewall states (Drop existing connections)

I am looking for a more comprehensive FreeBSD or pfSense network fault-injection toolkit, to test:

  • Introducing network delays
  • Introduce intermittent dropped packets
  • HTTP-based error injection

If this doesn't exist we have to implement something ourselves, like saboteur.

Copy link
Collaborator

@mediaminister mediaminister left a comment

Choose a reason for hiding this comment

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

I don't think we need to add rather technical error messages to a small Kodi ok dialog when it also states to look at the Kodi log for more information.

I'm not against implementing a network fault-injection toolkit, but personally I think logging error messages suffices because the add-on will fail anyway when network problems occur.

@dagwieers
Copy link
Collaborator Author

dagwieers commented Sep 2, 2020

I think we do.

Why ask for the user to go and look in the Kodi log if we basically know the problem is: "Connection refused" or "Name or service not found"? These small hints can pinpoint much better what could be the cause, or relate recurring issues. Most users will never go look into the logs when it fails.

My plan was actually to translate the specific error in a more user-friendly advice in the future, but we will always need to include the specific error message IMO.

PS The practical issue we do have is that an ok-dialog has only 4 visible lines shown, and while it can scroll it looks quite messy if you provide it with e.g. 8 lines of output.

@mediaminister
Copy link
Collaborator

To be clear, I'm totally okay with providing more detailed user-friendly information in a kodi ok dialog if the message fits 4 visible lines and respects the language choice of the user. In that case we don't need to ask the user to look at the log.

@dagwieers
Copy link
Collaborator Author

Well, the hint cannot be translated, but I am not sure it needs to be. This is what people may be looking for in issues, so it gives a tangible item that relates to the problem they are having. And at some point we may have a Wiki page with more elaborate info on what could be the problem.

It does fit in 4 lines currently (but depends on the message, luckily the network-related messages are all quite short).
And I put it in yellow so it stands out a bit, makes it less obtrusive. Could be in another color.

Maybe we could discuss with upstream to have less messy ok-dialogs.

@mediaminister
Copy link
Collaborator

4 visible lines should be enough to show an error message.

If a user has selected Dutch as the GUI language, I don't think it's very nice to show a message in English and expect him to understand what it means. Showing an error code that can be reported to the developers is also an option.

Showing less info and adding "Please try again later" is also an option to deal with unavoidable network errors in Kodi GUI.

Feel free to merge this when you think this is ready.

@dagwieers
Copy link
Collaborator Author

@mediaminister So, my preference would definitely be to have more specific error messages where we know the cause of the problem, or at least can pinpoint it to something specific. But it may not be as easy to pinpoint the cause, even for the ones below:

  • Name or service not known
    • Explanation: This would indicate either a local DNS issue, or an Internet access problem.
    • Resolution: Retry, if the problem persists, check your DNS configuration, or your Internet connection.
  • Connection refused
    • Explanation: This would indicate either a local firewall issue, or a major issue at VRT NU.
    • Resolution: Try the vrt.nu website, check your local firewall, check facebook or github issues.
  • No route to host
    • Explanation: This would indicate a local network issue, or a major issue at VRT NU.
    • Resolution: Try the vrt.nu website, check your local network configuration.
  • HTTP XXX issues

But we can never cover all possible issues, or offer a plausible explanation and resolution, so I prefer the fallback to include a bit more detail when we have it. If errors persist intermittently over a longer period this would make it easier to relate these issues together and give us some better understanding when we have to support these cases.

That the specific error is in English is in my opinion not as much a concern than not having it in the first place, even when it looks awkward.

@dagwieers
Copy link
Collaborator Author

Showing less info and adding "Please try again later" is also an option to deal with unavoidable network errors in Kodi GUI.

Yes, in general we should advise to:

  1. try again later
  2. try the vrtnu.be website or the VRT NU app
  3. check if there is a new add-on update available
  4. check facebook, twitter or github for known issues

@dagwieers
Copy link
Collaborator Author

dagwieers commented Sep 8, 2020

Another issue I just got. Starting a video from the recent menu (that was not up-to-date) return first this error:

screenshot041

followed by this error:

screenshot042

followed by this error:

screenshot043

And I could repeat it from the same menu. Refreshing the menu fixed this. It would be nice if this was just one error dialog.

Here is the log:

2020-09-09 00:50:12.527 T:1437520768  NOTICE: [plugin.video.vrt.nu] Access: plugin://plugin.video.vrt.nu/play/id/vid-6f784786-1fe2-40dd-8da9-7f3aeb19f3f7/pbs-pub-448e2beb-cf90-4ca4-aed2-40077f6e2b7a
2020-09-09 00:50:12.542 T:1437520768  NOTICE: [plugin.video.vrt.nu] Got item from cache '/storage/.kodi/userdata/addon_data/plugin.video.vrt.nu/tokens/ondemand_vrtPlayerToken.tkn'
2020-09-09 00:50:12.548 T:1437520768  NOTICE: [plugin.video.vrt.nu] URL get: https://media-services-public.vrt.be/vualto-video-aggregator-web/rest/external/v1/videos/pbs-pub-448e2beb-cf90-4ca4-aed2-40077f6e2b7a$vid-6f784786-1fe2-40dd-8da9-7f3aeb19f3f7?vrtPlayerToken=b10@999be590af49d20de05f1196b36f0e4027d0362f5f51cab769be6f9f0e67f715&client=vrtvideo@PROD
2020-09-09 00:50:43.631 T:1437520768   ERROR: [plugin.video.vrt.nu] HTTP Error 404: Not Found
2020-09-09 00:50:48.081 T:1936722368   ERROR: Playlist Player: skipping unplayable item: 0, path [plugin://plugin.video.vrt.nu/play/id/vid-6f784786-1fe2-40dd-8da9-7f3aeb19f3f7/pbs-pub-448e2beb-cf90-4ca4-aed2-40077f6e2b7a]

Update: Looked into why this was not correctly covered, and I am not sure what is the best way to not have these cascading errors.

play()get_stream()_get_stream_json()get_url_json()open_url()

We do not check the response immediately after returning from open_url(), and in get_stream() we enter another failure block causing the second dialog related to not having a VRT NU account (which is the wrong error in this case).

I started injecting failures to Kodi and this makes it handle them better.
@dagwieers dagwieers force-pushed the error-injection branch 2 times, most recently from 08ba8f5 to 7b34198 Compare September 9, 2020 01:07
This prevents the add-on from entering an empty virtual directory when
the request could not be handled. We may want to do this for other
specific HTTP errors...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants