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 warning if requests take longer then 10 seconds to process #36

Closed
Stevenic opened this issue Jan 26, 2018 · 7 comments
Closed

Add warning if requests take longer then 10 seconds to process #36

Stevenic opened this issue Jan 26, 2018 · 7 comments

Comments

@Stevenic
Copy link
Contributor

The bot has roughly 10-12 seconds to respond to a received activity before the channel times out. A channel timing out can result in some cases the message being re-delivered and in the case of cortana the skill being prematurely ended.

This feature would add a piece of middleware or sample that would log out that the bot is taking too long and the developer should investigate why. It could for instance log a warning for requests longer than 8 seconds and an error for requests longer then 12 seconds.

@garypretty
Copy link
Contributor

Once @cleemullins middleware M2 changes are in I am happy to pick this up and add a piece of middleware that can achieve the above (I am also working on ShowTyping, CatchException and ActivityFilter so makes sense that I pick this up as part of this - I have these ready, I am just waiting for the M2 bits to be merged). I propose we make the timeout period before the warning configurable when the middleware is added, with the defaults set to 8 second warning and 12 second error. @Stevenic how does that sound if you were to implement the equivalent in Node?

@garypretty
Copy link
Contributor

@cleemullins @Stevenic Also, I am planning to write a generic ConversationTimeout middleware which can allow the dev to prompt the user to continue or restart the conversation if they have not interacted with the bot for a given period of time. This needs the state changes to be considered first - plus thought into how the developer can indicate if the prompt is required (i.e. it only makes sense if the user is in the middle of a conversation flow before the timeout). Ill put some thought into this and report back, but it feels like this is something that is a common enough requirement to be added to the core extensions.

@Stevenic
Copy link
Contributor Author

@garypretty the configurable warning and error sounds reasonable. In fact you could probably just get away with logging the warning.

@garypretty the I'm not sure about putting your ConversationTimeout middleware into core but it definitely sounds useful. My hesitation around putting it into core is that it's planning to do dialog with the user which is something we've been trying to make the core components avoid.

@garypretty
Copy link
Contributor

@Stevenic I get the general rule of not putting dialog with the user into middleware, but I think this might be an exception as I think middleware suits this scenario perfectly. The plan would be a handler approach so the developer would be responsible for implementing the conversation and the middleware just for taking the timeout. Does that make sense? I think it is a common enough car for inclusion. Based on that does that change your thinking?

@Stevenic
Copy link
Contributor Author

Oh no... I think it's a good scenario I just meant that we've been avoiding putting that type of middleware into our core SDK.

I think letting the developer provide a handler is fine but I wonder how you would deal with the multi-turn nature of asking the user if they want to continue. You somehow need to tell the middleware plugin that you're in a conversation with the user and they should call your handler on the next turn. You'd also want to remember the original activity and play it back should they say "yes". You could do all of that fairly easily if the dialog is internal to the plugin but its the dance between the plugin and the devs handler that seems tricky.

@garypretty
Copy link
Contributor

Yes, when you mention the multi turn nature that does make sense. I'll stick with the exception handler, show typing and activity filter middleware for the core extensions for now and maybe create a sample in my own repo for the conversation timeout then. Great to chat that through 👍

@cleemullins
Copy link
Contributor

There's a PR sitting out there for this, but we decided to put this (for now) in a Toybox or similar - not in the core product.

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

No branches or pull requests

3 participants