Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.03k stars 1.19k forks source link

[Digital twins] DT Retry-After is not matching with core rest client #25647

Open KuNman opened 1 year ago

KuNman commented 1 year ago

Describe the bug Hey. While working with digital twins client and trying to execute few queries at same time I expected that options passed to DigitalTwinsClient:

retryOptions: {
  maxRetries: 20,
  retryDelayInMs: 5000,
},

don't change anything. My queries got immediate rejected with QuotaExceptionError. After some investigation of your code I spot that also there is no Retry-After header in response as mentioned in the docs. I found it in @azure/core-rest-pipeline/dist/index.js@getRetryAfterInMs.

if (!(response && [429, 503].includes(response.status)))
  return undefined;

Status code is 200 for QuotaExceptionError.

const retryAfterHeader = response.headers.get(RetryAfterHeader);

There is no "retry-after-ms", "x-ms-retry-after-ms", "Retry-After" in response.

To Reproduce Steps to reproduce the behavior:

  1. Execute few DT queries using digitalTwinsClient.queryTwins() till you will get QuotaExceptionError.

Expected behavior Retry logic is working.

Screenshots Here is function which I modified.

Screenshot 2023-04-24 at 23 46 02

Additional context Add any other context about the problem here.

xirzec commented 1 year ago

Looks like an issue with the service sending the wrong HTTP code here.

github-actions[bot] commented 1 year ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @sourabhguha @inesk-vt.

KuNman commented 1 year ago

Looks like an issue with the service sending the wrong HTTP code here.

Yes, response from DT backend is 200, not 429 for QuotaLimitException. I can understand why, but should be handled by http client then.

xirzec commented 1 year ago

@KuNman You're right that we can try to work around this in the client, but in general we try to avoid fixing poor API design at the client level since some consumers use raw REST calls or third-party libraries to make requests. Pragmatically we might have to do something like that in this case, but I'd like to hear from the service team before we invest in workarounds.