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

New skill handling support in botbuilder JS SDK with Directline Speech #1655

Closed
alopix opened this issue Feb 3, 2020 · 5 comments · Fixed by #1783
Closed

New skill handling support in botbuilder JS SDK with Directline Speech #1655

alopix opened this issue Feb 3, 2020 · 5 comments · Fixed by #1783
Assignees
Labels
Bot Services Required for internal Azure reporting. Do not delete. Do not change color. bug Indicates an unexpected problem or an unintended behavior. customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. customer-reported Issue is created by anyone that is not a collaborator in the repository.

Comments

@alopix
Copy link
Contributor

alopix commented Feb 3, 2020

Versions

botbuilder 4.7
node 10.16.3
macOS 10.14

Describe the bug

TL;DR;
It looks like the botbuilder-skills package is no longer being maintained, and instead similar functionality has been rolled into the botbuilder package. However, this new functionality does not appear to support Direct Line Speech out of the box - is there a timescale for that?

Full Description:
We discovered that the previous botbuilder-skills SDK seems to have been deprecated or at least not been worked on in a while and skill support been added to the main botbuilder JS SDK. To date we have been depending on the botbuilder-skills package.

Evaluating the changes necessary to migrate we noticed, that the example (https://github.com/microsoft/BotBuilder-Samples/tree/master/samples/javascript_nodejs/80.skills-simple-bot-to-bot) is specifically targeting DirectLine, as does the SDK itself. Support for DirectLine Speech is only possible with quite a bit of additional work as the way the adapter is being handled is not the same.

As there's not a single BotFrameworkAdapter but one for every WebSocket connection, it would also mean the new SkillHandler as well as ChannelServiceRoutes have to exist multiple times, which results in every conversation/connection resulting in a different endpoint via the SkillHandler (skillEndpoint.register(server, '/api/skills');).
Which currently is not easy to cleanup as the WebSocket does not seem to expose a disconnected event. This would result in a memory leak.

Apart from that, the solution does not necessarily scale with DirectLine Speech and multiple bot instances. A skill would only know the load balancer's URL and therefore might post back the response to an incorrect instance of the bot, which does not actually maintain the open client WebSocket connection.

A possible workaround for that would be some kind of message queue between skill and skill handler to distribute the responses – which is quite a bit of additional work.

I wonder if there's a different scaling solution Microsoft had in mind when designing this new skill architecture.

[bug]

@CoHealer CoHealer added Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-reported Issue is created by anyone that is not a collaborator in the repository. labels Feb 3, 2020
@daveta
Copy link
Contributor

daveta commented Feb 3, 2020

@darrenj : Can you comment?

@stevengum stevengum self-assigned this Feb 3, 2020
@daveta daveta removed Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-reported Issue is created by anyone that is not a collaborator in the repository. labels Feb 4, 2020
@clearab clearab self-assigned this Feb 4, 2020
@daveta daveta added Bot Services Required for internal Azure reporting. Do not delete. Do not change color. customer-reported Issue is created by anyone that is not a collaborator in the repository. labels Feb 5, 2020
@cleemullins
Copy link
Contributor

Hi @alopix Thanks for the details.

We're releasing a new JS build today (4.7.2, currently on our daily build server) to fix a bug around closed web sockets and unhandled exceptions. This is being regressed now, for release (hopefully) in the next 24 hours.

Regarding the concerns around websocket cleanup, I've moved that to a separate issue to track. We'll pick that up in R8.

@cleemullins
Copy link
Contributor

@alopix, can you elaborate on the config you're using with Web Sockets and DirectLine-Speech? Once I know more about your scenario & config, I can find the right person to address your horizontal scaling question.

@alopix
Copy link
Contributor Author

alopix commented Feb 7, 2020

As commented on the other new ticket:

Giving feedback on the linked ticket:
The setup is basically a combination of the two samples:

server.on('upgrade', async (req, socket, head) => {
    // Create an adapter scoped to this WebSocket connection to allow storing session data.
    const streamingAdapter = new BotFrameworkAdapter(adapterSettings);
    // Set onTurnError for the BotFrameworkAdapter created for each connection.
    streamingAdapter.onTurnError = onTurnErrorHandler;

    await streamingAdapter.useWebSocket(req, socket, head, async (context) => {
        // After connecting via WebSocket, run this logic for every request sent over
        // the WebSocket connection.
        await myBot.run(context);
    });
});

combined with (both top-level)

// Create and initialize the skill classes
const authConfig = new AuthenticationConfiguration([], allowedSkillsClaimsValidator);
const handler = new SkillHandler(adapter, bot, conversationIdFactory, credentialProvider, authConfig);
const skillEndpoint = new ChannelServiceRoutes(handler);
skillEndpoint.register(server, '/api/skills');

Would not work, as SkillHandler requires access to the adapter, which is only created on connection -> ChannelServiceRoutes and registration with the server would have to be inside the websocket connection handler as well, so requires a different endpoint all the time.

@clearab clearab added the customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. label Feb 8, 2020
@carlosscastro
Copy link
Member

Update: We'll be having a number of discussions on this early next week and we expect to come up with a plan after them. Expect a relevant update and plan mid next week.

@github-actions github-actions bot added the bug Indicates an unexpected problem or an unintended behavior. label Feb 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bot Services Required for internal Azure reporting. Do not delete. Do not change color. bug Indicates an unexpected problem or an unintended behavior. customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. customer-reported Issue is created by anyone that is not a collaborator in the repository.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants