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

feat: implement skipping backoff strategy #177

Merged
merged 10 commits into from
Nov 23, 2023

Conversation

daveleek
Copy link
Collaborator

@daveleek daveleek commented Nov 23, 2023

Description

Implements a backoff strategy for metrics and features endpoints based on skipping scheduled polls/posts.
Specific failed request responses (401, 403, 404, 429, 500, 502, 503, 504) increases amount of requests to skip.
10 skips in a row is maximum.
401, 403, 404 cause the skip counter to increase to 10 immediately

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Unit tests to provoke the behavior has been added

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@daveleek daveleek changed the title Feat/response statuscode handling feat: implement skipping backoff strategy Nov 23, 2023
}

[Test]
public void Access_Error_Responses_Goes_Straight_To_Ten()
Copy link
Member

Choose a reason for hiding this comment

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

Just looking at the test name, I don't see a check for responses == 10? Is that intentional? Am I reading the test wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah can't check it directly, so there's a loop of 10 in there along with a check that call count hasn't increased as a result of that loop - indicating it went straight to skipping 10, and then the additional invocation that should be let through the skip check and increase call count by 1

…koff_Tests.cs

Co-authored-by: Simon Hornby <liquidwicked64@gmail.com>
Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Don't know the feature very well but overall I think this LGTM. I would probably wait for a review from someone who's actually implemented this one tho

var client = GetClient(messageHandler);

// Wind up call count to 10
for (var i = 0; i < 55; i++)
Copy link
Member

Choose a reason for hiding this comment

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

55?

Copy link
Member

Choose a reason for hiding this comment

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

oh, right SUM(1..10) for the skips.

Copy link
Member

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

Nice. Looks recognizable :)

@daveleek daveleek merged commit 1bf92a5 into main Nov 23, 2023
1 check passed
@daveleek daveleek deleted the feat/response-statuscode-handling branch November 23, 2023 13:42
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