Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

feat(icon): Retry loading hiccup resources. #2172

Merged

Conversation

piotrl
Copy link
Contributor

@piotrl piotrl commented Jul 5, 2021

  • internal issue: APM-310496 APM-282779
  • nowadays CDNs are distributed systems that does not guarantees your request to reach right place. It started happening few hounders times per day for us that some icon couldn't be loaded
  • CloudFront explicitly marks that requests needs to be retried
  • mechanism is fairly simple, we retry any error from the server side (AWS throws 500 / 504 / 503 / 403)
    • 3 retries is enough for other resources we already handle, it should be fine here too
  • docs: https://www.learnrxjs.io/learn-rxjs/operators/error_handling/retry

Type of PR

Feature (non-breaking change which adds functionality)

Checklist

  • [*] I have read the CONTRIBUTING doc and I follow the PR guidelines
  • [*] Lint and unit tests pass locally with my changes
  • [ *] I have added tests that prove my fix is effective or that my feature works
  • [ *] I have added necessary documentation (if appropriate)

- internal issue: APM-310496 APM-282779
- nowadays CDNs are distributed systems that does not guarantees your request to reach right place. It started happening few hounders times per day for us that some icon couldn't be loaded
- CloudFront explicitly marks that requests *needs* to be retried
- mechanism is fairly simple, we retry any error from the server side (AWS throws 500 / 504 / 503 / 403)
  - 3 retries is enough for other resources we already handle
- docs: https://www.learnrxjs.io/learn-rxjs/operators/error_handling/retry
@piotrl piotrl requested a review from thomaspink as a code owner July 5, 2021 14:44
@dyladan
Copy link
Member

dyladan commented Jul 5, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@tomheller tomheller left a comment

Choose a reason for hiding this comment

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

Awesome addition, thank you @piotrl

🎉

@tomheller tomheller added the pr: merge-ready This PR is ready to be merged label Jul 6, 2021
@rowa-audil rowa-audil added pr: merge-ready This PR is ready to be merged pr: needs-cherry-pick When a pull request needs manual cherry picking and removed pr: merge-ready This PR is ready to be merged labels Jul 7, 2021
Copy link
Contributor

@thomaspink thomaspink left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you

@nimrod13 nimrod13 merged commit f73b9f3 into dynatrace-oss:master Jul 12, 2021
piotrl added a commit to piotrl/barista that referenced this pull request Sep 23, 2021
* Previous fix PR dynatrace-oss#2172 improved spam a lot, we have now instead 500 logs daily, only 1-2
* But still we have them daily, preferrably we'd like to not see it again as a hiccup ;)
* Suggestion: Keep severity as INFO instead WARN
thomaspink pushed a commit that referenced this pull request Sep 23, 2021
* Previous fix PR #2172 improved spam a lot, we have now instead 500 logs daily, only 1-2
* But still we have them daily, preferrably we'd like to not see it again as a hiccup ;)
* Suggestion: Keep severity as INFO instead WARN
thomaspink pushed a commit that referenced this pull request Sep 23, 2021
* Previous fix PR #2172 improved spam a lot, we have now instead 500 logs daily, only 1-2
* But still we have them daily, preferrably we'd like to not see it again as a hiccup ;)
* Suggestion: Keep severity as INFO instead WARN
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: needs-cherry-pick When a pull request needs manual cherry picking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants