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

Port WebResource keepAlive setting from 2.x to 1.x #402

Merged
merged 4 commits into from
Oct 1, 2020
Merged

Port WebResource keepAlive setting from 2.x to 1.x #402

merged 4 commits into from
Oct 1, 2020

Conversation

joshgummersall
Copy link
Contributor

This port will allow the Bot Framework SDK to better support customer bots with high traffic.

For some more context, the Bot Framework JS SDK currently makes heavy use of the @azure/ms-rest-js library. Due to requirements around backwards compatibility, we are tied to the 1.x branch (currently v1.8.15). In particular, we support customization of the underlying Axios HTTP client. In effect, this means that upgrading to the 2.x version of this library is currently not tenable.

In order to better support customers with high traffic bots running on Azure, we need to leverage HTTP keepalive as much as possible. The 2.x version of this library added a keepAlive setting to the WebResource class (though, to be fair, exposing an agent parameter directly might be even more friendly). This PR ports this functionality back to the 1.x branch and uses a shared pair of http/https agents so that HTTP connections can be shared across instances of the HTTP client (similar to the shared axios instance).

With this change, the Bot Framework SDK can support enabling keepAlive using a request policy:

import { HttpOperationResponse, RequestPolicy, ServiceClientOptions, WebResource } from '@azure/ms-rest-js';

const options: ServiceClientOptions = {};

const create = (nextPolicy: RequestPolicy): RequestPolicy => {
    const sendRequest = (httpRequest: WebResource): Promise<HttpOperationResponse> => {
        httpRequest.keepAlive = true;
        return nextPolicy.sendRequest(httpRequest);
    };

    return { sendRequest };
};

options.requestPolicyFactories = (factories) => [...factories, { create }];

Here is a sample issue on the Bot Framework SDK repo that illustrates that this is a pain point for current users.

I'm happy to put in more legwork here if this change seems reasonable.

Moreover, if supporting a ServiceClientOptions.agentSettings option seems reasonable I am happy to work on that on both the 2.x and 1.x branches. I think this could be a really useful feature because it would allow users of the library to have complete control over the HTTP(S) Agent used for outbound requests. In fact, I was 85% done writing that code on the 1.x branch before I realized I ought to mimic the behavior added to the 2.x release first.

This port will allow the botframework JS SDK to better support customer
bots with high traffic.
Copy link

@stevengum stevengum left a comment

Choose a reason for hiding this comment

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

This change will help a lot of customers, thanks for creating the PR!

Moreover, if supporting a ServiceClientOptions.agentSettings option seems reasonable I am happy to work on that on both the 2.x and 1.x branches. I think this could be a really useful feature because it would allow users of the library to have complete control over the HTTP(S) Agent used for outbound requests.

I agree that surfacing a way for users to configure the Agent or provide their own Agent would be amazing. Users providing their own Agent would also lower the surface area of @azure/ms-rest-js because it no longer needs to manually plumb through proxy configurations and keepAlive values.

@carlosscastro
Copy link

This change would be incredibly impactful to improve the development experience of Bot Framework SDK developers!!!

Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

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

Looks good! The CI error is related to some old checks used in this repo, unrelated to this change. I'll ship the update once CI on master completes.

@daviwil daviwil merged commit 808ceee into Azure:1.x Oct 1, 2020
@joshgummersall
Copy link
Contributor Author

Awesome, thanks for the quick review & merge @daviwil!

@daviwil
Copy link
Contributor

daviwil commented Oct 1, 2020

Strangely the CI automation is failing on 1.x branch, might be tomorrow before the update goes out if I have to fix anything. I'll keep you posted here.

@joshgummersall
Copy link
Contributor Author

Sounds good, thanks.

@EricDahlvang
Copy link

This is great! thanks @joshgummersall

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

Successfully merging this pull request may close these issues.

5 participants