Skip to content

Conversation

@ahmedetefy
Copy link
Contributor

@ahmedetefy ahmedetefy commented Apr 21, 2021

This PR:

  • Implement X-Sentry-Rate-Limits handling for node SDK transports
  • Add rate limiting based on SentryRequestType category -> event, transaction, session
  • If no categories are sent in the X-Sentry-Rate-Limits header -> all categories are rate limited
  • If bogus headers are sent with missing categories -> all categories are also rate limited
  • If bogus or unsupported categories are sent in the header, these categories are just ignored

@ahmedetefy ahmedetefy force-pushed the ahmed-ratelimit-node branch from 12b0628 to 2af71f0 Compare April 21, 2021 15:24
@ahmedetefy ahmedetefy marked this pull request as ready for review April 21, 2021 15:25
@ahmedetefy ahmedetefy requested a review from kamilogorek as a code owner April 21, 2021 15:25
@github-actions
Copy link
Contributor

github-actions bot commented Apr 21, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.63 KB (0%)
@sentry/browser - Webpack 21.5 KB (0%)
@sentry/react - Webpack 21.53 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 27.92 KB (+0.01% 🔺)

Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

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

Perfect copy-paste skills 🥰

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

I think the only thing missing is bogus test cases where the header is malformed or not there.
Also what about an unknown category?

https://github.com/getsentry/sentry-cocoa/blob/master/Tests/SentryTests/Networking/RateLimits/SentryRateLimitsParserTests.swift

@ahmedetefy ahmedetefy force-pushed the ahmed-ratelimit-node branch from 2af71f0 to fa33eb1 Compare May 5, 2021 10:38
@ahmedetefy ahmedetefy requested a review from HazAT May 5, 2021 10:41
@ahmedetefy ahmedetefy force-pushed the ahmed-ratelimit-node branch from fa33eb1 to 7e3e604 Compare May 5, 2021 11:12
Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

Do we really need to test both http and https and have a lot of duplicated tests? I think for this PR that's not necessary.

Add Category rate limiting based on SentryRequestType
@ahmedetefy ahmedetefy force-pushed the ahmed-ratelimit-node branch from 7e3e604 to c9978cd Compare May 6, 2021 11:37
@ahmedetefy ahmedetefy requested review from HazAT and rhcarvalho May 6, 2021 11:38
@HazAT HazAT merged commit 0f75bd7 into master May 6, 2021
@HazAT HazAT deleted the ahmed-ratelimit-node branch May 6, 2021 14:27
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.

5 participants