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

[#IOPSC-118] Respond with 'too many requests' status code when creating too much subscriptions for the same account #202

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

balanza
Copy link
Contributor

@balanza balanza commented Dec 9, 2022

List of Changes

See commit list

Motivation and Context

Azure API Management may fail to create too many subscriptions for the same account. According to Microsoft support, this is due to its internal optimistic consistency mechanism. Such failures do not depend on us or on a specific traffic pattern and are non-deterministic from our perspective; nevertheless, they are treated as generic server errors 500 and are hard to be monitored apart and react to differently.

To improve our users' experience we decided to:

  • retry up to 3 times on the subscriptions.createOrUpdate operation when a 412 error is returned, so our users do not have to care about anything at the cost of a slightly-higher response time
  • respond with 429 if retries do not solve, so that they can regulate request rate themselves (it will also be helpful for us as we can better monitor failures)

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Chore (nothing changes from a user perspective)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@balanza balanza requested a review from a team as a code owner December 9, 2022 18:37
@balanza balanza requested review from BurnedMarshal and domenicoventriglia and removed request for a team December 9, 2022 18:37
@balanza balanza changed the title [#IOPSC-118] Respond with _too many requests_ status code when creating too much subscriptions for the same account [#IOPSC-118] Respond with 'too many requests' status code when creating too much subscriptions for the same account Dec 9, 2022
@balanza balanza requested review from LoFrance and iwoak and removed request for domenicoventriglia December 9, 2022 18:38
@pagopa-github-bot
Copy link
Contributor

Warnings
⚠️

Please include a Pivotal story at the beginning of the PR title (see below).

Example of PR titles that include pivotal stories:

  • single story: [#123456] my PR title
  • multiple stories: [#123456,#123457,#123458] my PR title

Generated by 🚫 dangerJS

@balanza
Copy link
Contributor Author

balanza commented Dec 9, 2022

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@balanza balanza enabled auto-merge (squash) December 12, 2022 15:52
@balanza balanza disabled auto-merge December 12, 2022 15:52
@balanza balanza merged commit 3e9bc25 into master Dec 12, 2022
@balanza balanza deleted the IOPSC-118--retry branch December 12, 2022 15:52
Comment on lines +178 to +188
if (error === null) {
return false;
}
if (!(error instanceof RestError)) {
return false;
}
if (!error.statusCode) {
return false;
}

return error.statusCode === statusCode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not in fp-ts?

flow(
    E.fromPredicate(
      () =>
        error instanceof RestError &&
        error.statusCode === statusCode,
     () => false
    ),
    E.fold(
      identity,
      (_) => true
    )
  )(error);

@balanza balanza mentioned this pull request Dec 13, 2022
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.

3 participants