Skip to content

Commit

Permalink
[Text Analytics] Fix rate limit reached issue (#19249)
Browse files Browse the repository at this point in the history
* [Text Analytics] Fix rate limit reached issue

* create an options bag for the throttling retry policy

* use the public options to configure the max retries instead

* simplify the retry options type

* Update sdk/textanalytics/ai-text-analytics/test/public/utils/recordedClient.ts

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

* edit the comment

* Update sdk/core/core-rest-pipeline/CHANGELOG.md

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>
  • Loading branch information
deyaaeldeen and xirzec authored Dec 14, 2021
1 parent 12b1941 commit f31aa42
Show file tree
Hide file tree
Showing 11 changed files with 89 additions and 26 deletions.
3 changes: 2 additions & 1 deletion sdk/core/core-rest-pipeline/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
# Release History

## 1.3.3 (Unreleased)
## 1.4.0 (Unreleased)

### Features Added

- The `bearerTokenAuthenticationPolicy` now accepts a logger.
- Changed behavior when sending HTTP headers to preserve the original casing of header names. Iterating over `HttpHeaders` now keeps the original name casing. There is also a new `preserveCase` option for `HttpHeaders.toJSON()`. See [PR #18517](https://github.com/Azure/azure-sdk-for-js/pull/18517)
- The count for how many retries in the `throttlingRetryPolicy` policy can now be configured.

### Breaking Changes

Expand Down
2 changes: 1 addition & 1 deletion sdk/core/core-rest-pipeline/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@azure/core-rest-pipeline",
"version": "1.3.3",
"version": "1.4.0",
"description": "Isomorphic client library for making HTTP requests in node.js and browser.",
"sdk-type": "client",
"main": "dist/index.js",
Expand Down
16 changes: 14 additions & 2 deletions sdk/core/core-rest-pipeline/review/core-rest-pipeline.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ export interface Pipeline {
export interface PipelineOptions {
proxyOptions?: ProxySettings;
redirectOptions?: RedirectPolicyOptions;
retryOptions?: ExponentialRetryPolicyOptions;
retryOptions?: PipelineRetryOptions;
userAgentOptions?: UserAgentPolicyOptions;
}

Expand Down Expand Up @@ -242,6 +242,13 @@ export interface PipelineResponse {
status: number;
}

// @public
export interface PipelineRetryOptions {
maxRetries?: number;
maxRetryDelayInMs?: number;
retryDelayInMs?: number;
}

// @public
export function proxyPolicy(proxySettings?: ProxySettings | undefined, options?: {
customNoProxyList?: string[];
Expand Down Expand Up @@ -323,11 +330,16 @@ export interface SystemErrorRetryPolicyOptions {
}

// @public
export function throttlingRetryPolicy(): PipelinePolicy;
export function throttlingRetryPolicy(options?: ThrottlingRetryPolicyOptions): PipelinePolicy;

// @public
export const throttlingRetryPolicyName = "throttlingRetryPolicy";

// @public
export interface ThrottlingRetryPolicyOptions {
maxRetries?: number;
}

// @public
export function tracingPolicy(options?: TracingPolicyOptions): PipelinePolicy;

Expand Down
10 changes: 4 additions & 6 deletions sdk/core/core-rest-pipeline/src/createPipelineFromOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@
// Licensed under the MIT license.

import { ProxySettings } from ".";
import { PipelineRetryOptions } from "./interfaces";
import { Pipeline, createEmptyPipeline } from "./pipeline";
import { decompressResponsePolicy } from "./policies/decompressResponsePolicy";
import {
exponentialRetryPolicy,
ExponentialRetryPolicyOptions
} from "./policies/exponentialRetryPolicy";
import { exponentialRetryPolicy } from "./policies/exponentialRetryPolicy";
import { formDataPolicy } from "./policies/formDataPolicy";
import { logPolicy, LogPolicyOptions } from "./policies/logPolicy";
import { proxyPolicy } from "./policies/proxyPolicy";
Expand All @@ -27,7 +25,7 @@ export interface PipelineOptions {
/**
* Options that control how to retry failed requests.
*/
retryOptions?: ExponentialRetryPolicyOptions;
retryOptions?: PipelineRetryOptions;

/**
* Options to configure a proxy for outgoing requests.
Expand Down Expand Up @@ -72,7 +70,7 @@ export function createPipelineFromOptions(options: InternalPipelineOptions): Pip
pipeline.addPolicy(tracingPolicy(options.userAgentOptions));
pipeline.addPolicy(userAgentPolicy(options.userAgentOptions));
pipeline.addPolicy(setClientRequestIdPolicy());
pipeline.addPolicy(throttlingRetryPolicy(), { phase: "Retry" });
pipeline.addPolicy(throttlingRetryPolicy(options.retryOptions), { phase: "Retry" });
pipeline.addPolicy(systemErrorRetryPolicy(options.retryOptions), { phase: "Retry" });
pipeline.addPolicy(exponentialRetryPolicy(options.retryOptions), { phase: "Retry" });
pipeline.addPolicy(redirectPolicy(options.redirectOptions), { afterPhase: "Retry" });
Expand Down
9 changes: 7 additions & 2 deletions sdk/core/core-rest-pipeline/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ export {
RawHttpHeaders,
RawHttpHeadersInput,
TransferProgressEvent,
RequestBodyType
RequestBodyType,
PipelineRetryOptions
} from "./interfaces";
export {
AddPolicyOptions as AddPipelineOptions,
Expand Down Expand Up @@ -58,7 +59,11 @@ export {
SystemErrorRetryPolicyOptions,
systemErrorRetryPolicyName
} from "./policies/systemErrorRetryPolicy";
export { throttlingRetryPolicy, throttlingRetryPolicyName } from "./policies/throttlingRetryPolicy";
export {
throttlingRetryPolicy,
throttlingRetryPolicyName,
ThrottlingRetryPolicyOptions
} from "./policies/throttlingRetryPolicy";
export { tracingPolicy, tracingPolicyName, TracingPolicyOptions } from "./policies/tracingPolicy";
export {
userAgentPolicy,
Expand Down
23 changes: 23 additions & 0 deletions sdk/core/core-rest-pipeline/src/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -294,3 +294,26 @@ export type FormDataValue = string | Blob;
* A simple object that provides form data, as if from a browser form.
*/
export type FormDataMap = { [key: string]: FormDataValue | FormDataValue[] };

/**
* Options that control how to retry failed requests.
*/
export interface PipelineRetryOptions {
/**
* The maximum number of retry attempts. Defaults to 10.
*/
maxRetries?: number;

/**
* The amount of delay in milliseconds between retry attempts. Defaults to 1000
* (1 second). The delay increases exponentially with each retry up to a maximum
* specified by maxRetryDelayInMs.
*/
retryDelayInMs?: number;

/**
* The maximum delay in milliseconds allowed before retrying an operation. Defaults
* to 64000 (64 seconds).
*/
maxRetryDelayInMs?: number;
}
19 changes: 16 additions & 3 deletions sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,17 @@ export const throttlingRetryPolicyName = "throttlingRetryPolicy";
/**
* Maximum number of retries for the throttling retry policy
*/
export const DEFAULT_CLIENT_MAX_RETRY_COUNT = 3;
export const DEFAULT_CLIENT_MAX_RETRY_COUNT = 10;

/**
* Options that control how to retry failed requests.
*/
export interface ThrottlingRetryPolicyOptions {
/**
* The maximum number of retry attempts. Defaults to 10.
*/
maxRetries?: number;
}

/**
* A policy that retries when the server sends a 429 response with a Retry-After header.
Expand All @@ -22,14 +32,17 @@ export const DEFAULT_CLIENT_MAX_RETRY_COUNT = 3;
* https://docs.microsoft.com/en-us/azure/azure-resource-manager/resource-manager-request-limits,
* https://docs.microsoft.com/en-us/azure/azure-subscription-service-limits and
* https://docs.microsoft.com/en-us/azure/virtual-machines/troubleshooting/troubleshooting-throttling-errors
*
* @param options - Options that configure retry logic.
*/
export function throttlingRetryPolicy(): PipelinePolicy {
export function throttlingRetryPolicy(options: ThrottlingRetryPolicyOptions = {}): PipelinePolicy {
return {
name: throttlingRetryPolicyName,
async sendRequest(request: PipelineRequest, next: SendRequest): Promise<PipelineResponse> {
let response = await next(request);
const maxRetryCount = options.maxRetries ?? DEFAULT_CLIENT_MAX_RETRY_COUNT;

for (let count = 0; count < DEFAULT_CLIENT_MAX_RETRY_COUNT; count++) {
for (let count = 0; count < maxRetryCount; count++) {
if (response.status !== 429 && response.status !== 503) {
return response;
}
Expand Down
14 changes: 8 additions & 6 deletions sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
createHttpHeaders,
throttlingRetryPolicy
} from "../src";
import { DEFAULT_CLIENT_MAX_RETRY_COUNT } from "../src/policies/throttlingRetryPolicy";

describe("throttlingRetryPolicy", function() {
afterEach(function() {
Expand Down Expand Up @@ -177,7 +178,7 @@ describe("throttlingRetryPolicy", function() {
clock.restore();
});

it("It should retry up to three times", async function(this: Context) {
it("It should retry up to the default max retries", async function(this: Context) {
const clock = sinon.useFakeTimers();

const request = createPipelineRequest({
Expand All @@ -193,11 +194,12 @@ describe("throttlingRetryPolicy", function() {

const policy = throttlingRetryPolicy();
const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
next.onCall(0).resolves(retryResponse);
next.onCall(1).resolves(retryResponse);
next.onCall(2).resolves(retryResponse);
let i = 0;
for (; i < DEFAULT_CLIENT_MAX_RETRY_COUNT; ++i) {
next.onCall(i).resolves(retryResponse);
}
// This one should be returned
next.onCall(3).resolves({
next.onCall(i).resolves({
headers: createHttpHeaders({
"Retry-After": "1",
"final-response": "final-response"
Expand All @@ -207,7 +209,7 @@ describe("throttlingRetryPolicy", function() {
});

const promise = policy.sendRequest(request, next);
await clock.tickAsync(3000);
await clock.tickAsync(i * 1000);
const response = await promise;
assert.equal(response.status, 503);
assert.equal(response.headers.get("final-response"), "final-response");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,20 +295,20 @@ describe("ManagedIdentityCredential", function() {

it("doesn't try IMDS endpoint again once it can't be detected", async function() {
const credential = new ManagedIdentityCredential("errclient");
const DEFAULT_CLIENT_MAX_RETRY_COUNT = 10;
const authDetails = await sendCredentialRequests({
scopes: ["scopes"],
credential,
insecureResponses: [
// Satisfying the ping
createResponse(200),
// Retries until exhaustion
createResponse(503, {}, { "Retry-After": "2" }),
createResponse(503, {}, { "Retry-After": "2" }),
createResponse(503, {}, { "Retry-After": "2" }),
createResponse(503, {}, { "Retry-After": "2" })
...Array(DEFAULT_CLIENT_MAX_RETRY_COUNT + 1).fill(
createResponse(503, {}, { "Retry-After": "2" })
)
]
});
assert.equal(authDetails.requests.length, 5);
assert.equal(authDetails.requests.length, DEFAULT_CLIENT_MAX_RETRY_COUNT + 2);
assert.ok(authDetails.error!.message.indexOf("authentication failed") > -1);

await testContext.restore();
Expand Down
3 changes: 3 additions & 0 deletions sdk/textanalytics/ai-text-analytics/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,9 @@ export function compileError(errorResponse: unknown): any {
};
statusCode: number;
};
if (!castErrorResponse.response) {
throw errorResponse;
}
const topLevelError = castErrorResponse.response.parsedBody?.error;
if (!topLevelError) return errorResponse;
let errorMessage = topLevelError.message || "";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,12 @@ matrix([["APIKey", "AAD"]] as const, async (authMethod: AuthMethod) => {
this.timeout(isPlaybackMode() ? fastTimeout : CLITimeout);
});

/**
* The service's rate limit is sometimes reached for the beginAnalyzeActions tests
* because of the many requests the tests send. It appears that the number of
* max retries should be big enough to get around this issue. 10 seems to work
* fine for now.
*/
describe("#analyze", function() {
it("single custom entity recognition action", async function() {
const docs = [
Expand Down

0 comments on commit f31aa42

Please sign in to comment.