-
Notifications
You must be signed in to change notification settings - Fork 276
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
Integrate AgentKeepAlive fix into botframework-connector #183
Comments
The challenge here is to get both proxy support and keep-alive working together. It seems that now you can have just one or the other. |
Guys, any news on this? I'm doing a benchmark with the partner and we are seeing 1/2 of the performance from a Node.js bot compared to a .NET Core one. If somebody can point me where this call is made I could try to implement the change to check if this is the cause of the performance difference I'm seeing and submit a PR. |
Hey @damadei Thank you for your willingness to help out with this issue. I'd like to work with you it, and I've emailed you offline. |
It seems that the reason BotBuilder-dotnet does not have these issues is due to the caching of ConnectorClients https://github.com/Microsoft/botbuilder-dotnet/blob/master/libraries/Microsoft.Bot.Builder/BotFrameworkAdapter.cs#L59 as well as using a static HttpClient https://github.com/Microsoft/botbuilder-dotnet/blob/master/libraries/Microsoft.Bot.Builder/BotFrameworkAdapter.cs#L886 These two patterns, or something similar, should also be implemented in BotBuilder-js. |
Now that we are off autorest, we can modify the code that could address this. moving out of 4.5 |
This is still blocked; the last release of the 1.x line of @azure/ms-rest-js does not contain a way to inject a custom Agent with keepAlive configured to true ( |
Actually, previous comment only applies for using the built-in Node.js HttpClient (AxiosHttpClient in 1.x of ms-rest-js). If #2013 is addressed and ServiceClientOptions is surfaced to developers, they would be able to provide their own HttpClient which could use their own HTTP request library with Agent support. |
With #2013 merged, it is now possible to provide a custom @ms-rest-js.HttpClient as a parameter to BotFrameworkAdapter: const adapter = new BotFrameworkAdapter({
appId: process.env.MicrosoftAppId,
appPassword: process.env.MicrosoftAppPassword,
clientOptions: { httpClient: new CustomAxiosHttpClient() }
}); However, this is still very inconvenient and cumbersome to implement. The AxiosHttpClient provided by the 1.x version of ms-rest-js contains some custom error handling, abort logic, stream support, etc. Much of this is not needed by the sdk, but in order to use agent keepAlive our recommendation for R9 will be to copy this file into the bot and modify axios.create() to include: httpAgent: new http.Agent({ keepAlive: true }),
httpsAgent: new https.Agent({ keepAlive: true }), We could make this easier. Lastly, we need to consider options for providing keepAlive agents to QnAMaker, LUIS, and storage providers. |
Hi Team, |
Currently, the sdk provides and const adapter = new BotFrameworkAdapter({
appId: process.env.MicrosoftAppId,
appPassword: process.env.MicrosoftAppPassword,
clientOptions: { httpClient: new CustomAxiosHttpClient() }
}); Here is a Unfortunately, the sdk does not currently expose an agent parameter for QnAMaker or Luis implementations which are using |
High volume azure hosted bots can start getting timeouts due to an SNAT issue. We have a blog post for how to fix this but I think we could build the fix into the connector such that's it easier to turn on and configure. I'm not sure it should be on by default but I'm all for making it easy to enable.
The text was updated successfully, but these errors were encountered: