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

chore(project): Add backoff mechanism for fetching projects #1726

Merged
merged 10 commits into from
Jan 11, 2023

Conversation

olksdr
Copy link
Contributor

@olksdr olksdr commented Dec 29, 2022

In case of getting invalid project we try to reuse the stale state and make sure to initiate the backoff to slow down the requests to the project cache, so we don't ddos out own service.

We reset the backoff only when the new state is accepted.

closes: #1633

@olksdr olksdr requested a review from a team December 29, 2022 10:55
@olksdr olksdr self-assigned this Dec 29, 2022
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Should we add a test for this behavior? Something like

def test_project_grace_period(mini_sentry, relay, grace_period):

// If the new state is valid or the old one is expired, always use the new one.
_ => self.state = Some(state.clone()),
// And also reset the backoff at this point.
_ => {
Copy link
Member

Choose a reason for hiding this comment

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

What happens when self.expiry_state() is Expired, but the new state is invalid()? Shouldn't we then also set a next_fetch_attempt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently (and the behaviour we had before) once the old state expires we save anything we get from upstream - this case the upstream is alive, just delivers the invalid state. Since we cannot use the old state either, we will just try to request the new one soon-ish again.
Do you think we have to re-think this behaviour in this PR?

@olksdr
Copy link
Contributor Author

olksdr commented Jan 10, 2023

Should we add a test for this behavior? Something like

def test_project_grace_period(mini_sentry, relay, grace_period):

Not sure about the test. I do not really want to introduce more "waits" in already slow integration tests.
But I can have a look into this if you think it would be better to have one.

@olksdr olksdr enabled auto-merge (squash) January 11, 2023 10:57
@olksdr olksdr merged commit 337a8a0 into master Jan 11, 2023
@olksdr olksdr deleted the chore/add-backoff-to-fetch-prjcts branch January 11, 2023 11:10
olksdr added a commit that referenced this pull request Jan 11, 2023
In case of getting invalid project we try to reuse the stale state and
make sure to initiate the backoff to slow down the requests to the
project cache, so we don't ddos out own service.

We reset the backoff only when the new state is accepted.
jan-auer added a commit that referenced this pull request Jan 18, 2023
* master: (35 commits)
  ref(actix): Migrate ProjectUpstream to `relay_system::Service` (#1727)
  feat(general): Add unknown SessionStatus variant (#1736)
  ref: Convert integration tests about dropping transactions to unit tests (#1720)
  release: 0.8.16
  ci: Skip redundant self-hosted E2E on library release (#1755)
  doc(changelog): Add relevant changes to python changelog (#1753)
  feat(profiling): Add profile context (#1748)
  release: 23.1.0
  profiling(fix): use an unpadded base64 encoding (#1749)
  Revert "feat(replays): Enable PII scrubbing for all organizations" (#1747)
  feat: Switch from base64 to data-encoding (#1743)
  instr(replays): Add timer metric to recording processing (#1742)
  feat(replays): Use Annotated struct definition for replay-event parsing (#1582)
  feat(sessions): Retire session duration metric (#1739)
  feat(general): Scrub all fields with IP address (#1725)
  feat(replays): Enable PII scrubbing for all organizations (#1678)
  chore(project): Add backoff mechanism for fetching projects (#1726)
  feat(profiling): Add new measurement units for profiling (#1732)
  chore(toolchain): update rust to 1.66.1 (#1735)
  ref(actix): Migrate server actor to the "service" arch (#1723)
  ...
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.

[INC-263] Backoff mechanism for fetching projects in case of error
2 participants