feat(replay): Stop recording when hitting a rate limit - #7018

Merged
Lms24 merged 2 commits into
developfrom
lms-replay-stop-on-ratelimit
Feb 1, 2023
Merged

feat(replay): Stop recording when hitting a rate limit#7018
Lms24 merged 2 commits into
developfrom
lms-replay-stop-on-ratelimit

Conversation

@Lms24

@Lms24Lms24 commented Feb 1, 2023•
edited
Loading

Copy link
Copy Markdown
Member

As discussed and outlined in #6984, we want to entirely stop recording the replay once we receive a 429 rate limit response. This PR gets rid of all the "pause on rate limit" logic. Because we already stop when we receive other http error responses, we can simply use this logic instead.

Note: feat() over ref() because it's a behaviour change, not because handling rate limits is entirely new (#6710)

closes#6984

@Lms24
Lms24 requested review from billyvg and mydeaFebruary 1, 2023 11:18
@Lms24
Lms24force-pushed the lms-replay-stop-on-ratelimit branch from d074416 to 4c2a170CompareFebruary 1, 2023 11:21
Lms24
Lms24 commented Feb 1, 2023
View reviewed changes
Comment threadpackages/replay/test/integration/rateLimiting.test.ts
replay && replay.stop();
});

it.each([

@Lms24Lms24Feb 1, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

IMO we don't need these tests anymore, as they only tested that we pause correctly for different rate limit timeouts we might receive from the Sentry backend. Just ensuring that we don't do anything after a 429 response should be good enough

@mydeamydeaFeb 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, makes sense! 👍 We could even move it to the general sendReplay tests I guess?

@Lms24Lms24Feb 1, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Generally I think it makes sense but given that we have this neat comment under the last test,

// NOTE: If you add a test after the last one, make sure to adjust the test setup// As this ends with a `stopped()` replay, which may prevent future tests from working// Sadly, fixing this turned out to be much more annoying than expected, so leaving this warning here for now

and that I ran exactly into this problem, wasting 30minutes trying fix it, I'd vote we leave it as is.

@mydeamydeaFeb 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ahh, right 🙈 damn we should fix those xD

@Lms24Lms24 changed the title rfe(replay): Stop recording when hitting a rate limitref(replay): Stop recording when hitting a rate limitFeb 1, 2023
@github-actions

github-actionsBot commented Feb 1, 2023

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.88 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.54 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.29 KB (0%)
@sentry/browser - Webpack (minified)66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.08 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)43.79 KB (-0.85% 🔽)
@sentry/replay - Webpack (gzipped + minified)38.7 KB (-0.5% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)61.38 KB (-0.21% 🔽)

mydea
mydea approved these changes Feb 1, 2023
View reviewed changes

@mydeamydea left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, saving some bytes!

l: We may want to move the tests to the general send replay tests, I guess we don't need a separate test module then. but this is super unimportant. Thanks for tackling this! 👍

@mydea

mydea commented Feb 1, 2023

Copy link
Copy Markdown
Member

One comment: I guess this should be feat, not ref, as it kind of changes the behavior? WDYT?

@Lms24Lms24 changed the title ref(replay): Stop recording when hitting a rate limitfeat(replay): Stop recording when hitting a rate limitFeb 1, 2023
@Lms24
Lms24 merged commit 6ddc5cd into developFeb 1, 2023
@Lms24
Lms24 deleted the lms-replay-stop-on-ratelimit branch February 1, 2023 16:25
@mydeamydea mentioned this pull request Oct 31, 2023
Merged
mydea added a commit that referenced this pull request Nov 2, 2023
@mydea
fix(replay): Ensure we stop for rate limit headers (#9420)
f796d50
We changed this
[here](#7018), but
apparently it is possible to have responses with a 200 status code but
rate limit headers.
This PR updates our handling to stop either for a non-200 status code,
_or_ for a rate limit header.
I also streamlined the tests for this a bit, we were testing a bunch of
unrelated things, IMHO it's enough to test we stopped/didn't stop.
Resolves: getsentry/sentry#49498
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
@billyvgbillyvgbillyvg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Stop replay recording when hitting a rate limit

3 participants

@Lms24@mydea@billyvg
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
feat(replay): Stop recording when hitting a rate limit - #7018
Merged
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Feb 1, 2023
Merged
feat(replay): Stop recording when hitting a rate limit#7018
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Conversation
@Lms24
@Lms24Lms24
commented
Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

Member
As discussed and outlined in #6984, we want to entirely stop recording the replay once we receive a 429 rate limit response. This PR gets rid of all the "pause on rate limit" logic. Because we already stop when we receive other http error responses, we can simply use this logic instead.
Note: feat() over ref() because it's a behaviour change, not because handling rate limits is entirely new (#6710)
closes#6984
@Lms24
Lms24
requested review from
billyvg and mydeaFebruary 1, 2023 11:18
@Lms24
Lms24force-pushed
the
lms-replay-stop-on-ratelimit
branch
from
d074416 to
4c2a170CompareFebruary 1, 2023 11:21
Lms24
Lms24
commented
Feb 1, 2023
View reviewed changes
Comment threadpackages/replay/test/integration/rateLimiting.test.ts
 replay && replay.stop();
 });

 it.each([
@Lms24Lms24Feb 1, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

IMO we don't need these tests anymore, as they only tested that we pause correctly for different rate limit timeouts we might receive from the Sentry backend. Just ensuring that we don't do anything after a 429 response should be good enough
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yes, makes sense! 👍 We could even move it to the general sendReplay tests I guess?
@Lms24Lms24Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Generally I think it makes sense but given that we have this neat comment under the last test,
// NOTE: If you add a test after the last one, make sure to adjust the test setup// As this ends with a `stopped()` replay, which may prevent future tests from working// Sadly, fixing this turned out to be much more annoying than expected, so leaving this warning here for now
and that I ran exactly into this problem, wasting 30minutes trying fix it, I'd vote we leave it as is.
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ahh, right 🙈 damn we should fix those xD
@Lms24Lms24
changed the title
rfe(replay): Stop recording when hitting a rate limitref(replay): Stop recording when hitting a rate limitFeb 1, 2023
@github-actions
github-actionsBot
commented
Feb 1, 2023

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.88 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.54 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.29 KB (0%)
@sentry/browser - Webpack (minified)66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.08 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)43.79 KB (-0.85% 🔽)
@sentry/replay - Webpack (gzipped + minified)38.7 KB (-0.5% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)61.38 KB (-0.21% 🔽)
mydea
mydea
approved these changes
Feb 1, 2023
View reviewed changes
@mydeamydea
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nice, saving some bytes!
l: We may want to move the tests to the general send replay tests, I guess we don't need a separate test module then. but this is super unimportant. Thanks for tackling this! 👍
@mydea
mydea
commented
Feb 1, 2023

Copy link


Copy Markdown

Member
One comment: I guess this should be feat, not ref, as it kind of changes the behavior? WDYT?
@Lms24Lms24
changed the title
ref(replay): Stop recording when hitting a rate limitfeat(replay): Stop recording when hitting a rate limitFeb 1, 2023
@Lms24
Lms24
merged commit 6ddc5cd
into
developFeb 1, 2023
@Lms24
Lms24
deleted the

lms-replay-stop-on-ratelimit

branch
February 1, 2023 16:25
@mydeamydea
mentioned this pull request
Oct 31, 2023
 Merged

mydea added a commit
that referenced
this pull request
Nov 2, 2023
@mydea
fix(replay): Ensure we stop for rate limit headers (#9420)
f796d50
We changed this
[here](#7018), but
apparently it is possible to have responses with a 200 status code but
rate limit headers.
This PR updates our handling to stop either for a non-200 status code,
_or_ for a rate limit header.
I also streamlined the tests for this a bit, we were testing a bunch of
unrelated things, IMHO it's enough to test we stopped/didn't stop.
Resolves: getsentry/sentry#49498
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
@billyvgbillyvgbillyvg approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Stop replay recording when hitting a rate limit


3 participants

@Lms24@mydea@billyvg

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(replay): Stop recording when hitting a rate limit - #7018
Merged
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Feb 1, 2023
Merged
feat(replay): Stop recording when hitting a rate limit#7018
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Conversation
@Lms24
@Lms24Lms24
commented
Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

Member
As discussed and outlined in #6984, we want to entirely stop recording the replay once we receive a 429 rate limit response. This PR gets rid of all the "pause on rate limit" logic. Because we already stop when we receive other http error responses, we can simply use this logic instead.
Note: feat() over ref() because it's a behaviour change, not because handling rate limits is entirely new (#6710)
closes#6984
@Lms24
Lms24
requested review from
billyvg and mydeaFebruary 1, 2023 11:18
@Lms24
Lms24force-pushed
the
lms-replay-stop-on-ratelimit
branch
from
d074416 to
4c2a170CompareFebruary 1, 2023 11:21
Lms24
Lms24
commented
Feb 1, 2023
View reviewed changes
Comment threadpackages/replay/test/integration/rateLimiting.test.ts
 replay && replay.stop();
 });

 it.each([
@Lms24Lms24Feb 1, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

IMO we don't need these tests anymore, as they only tested that we pause correctly for different rate limit timeouts we might receive from the Sentry backend. Just ensuring that we don't do anything after a 429 response should be good enough
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yes, makes sense! 👍 We could even move it to the general sendReplay tests I guess?
@Lms24Lms24Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Generally I think it makes sense but given that we have this neat comment under the last test,
// NOTE: If you add a test after the last one, make sure to adjust the test setup// As this ends with a `stopped()` replay, which may prevent future tests from working// Sadly, fixing this turned out to be much more annoying than expected, so leaving this warning here for now
and that I ran exactly into this problem, wasting 30minutes trying fix it, I'd vote we leave it as is.
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ahh, right 🙈 damn we should fix those xD
@Lms24Lms24
changed the title
rfe(replay): Stop recording when hitting a rate limitref(replay): Stop recording when hitting a rate limitFeb 1, 2023
@github-actions
github-actionsBot
commented
Feb 1, 2023

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.88 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.54 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.29 KB (0%)
@sentry/browser - Webpack (minified)66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.08 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)43.79 KB (-0.85% 🔽)
@sentry/replay - Webpack (gzipped + minified)38.7 KB (-0.5% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)61.38 KB (-0.21% 🔽)
mydea
mydea
approved these changes
Feb 1, 2023
View reviewed changes
@mydeamydea
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nice, saving some bytes!
l: We may want to move the tests to the general send replay tests, I guess we don't need a separate test module then. but this is super unimportant. Thanks for tackling this! 👍
@mydea
mydea
commented
Feb 1, 2023

Copy link


Copy Markdown

Member
One comment: I guess this should be feat, not ref, as it kind of changes the behavior? WDYT?
@Lms24Lms24
changed the title
ref(replay): Stop recording when hitting a rate limitfeat(replay): Stop recording when hitting a rate limitFeb 1, 2023
@Lms24
Lms24
merged commit 6ddc5cd
into
developFeb 1, 2023
@Lms24
Lms24
deleted the

lms-replay-stop-on-ratelimit

branch
February 1, 2023 16:25
@mydeamydea
mentioned this pull request
Oct 31, 2023
 Merged

mydea added a commit
that referenced
this pull request
Nov 2, 2023
@mydea
fix(replay): Ensure we stop for rate limit headers (#9420)
f796d50
We changed this
[here](#7018), but
apparently it is possible to have responses with a 200 status code but
rate limit headers.
This PR updates our handling to stop either for a non-200 status code,
_or_ for a rate limit header.
I also streamlined the tests for this a bit, we were testing a bunch of
unrelated things, IMHO it's enough to test we stopped/didn't stop.
Resolves: getsentry/sentry#49498
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
@billyvgbillyvgbillyvg approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Stop replay recording when hitting a rate limit


3 participants

@Lms24@mydea@billyvg

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(replay): Stop recording when hitting a rate limit - #7018
Merged
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Feb 1, 2023
Merged
feat(replay): Stop recording when hitting a rate limit#7018
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Conversation
@Lms24
@Lms24Lms24
commented
Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

Member
As discussed and outlined in #6984, we want to entirely stop recording the replay once we receive a 429 rate limit response. This PR gets rid of all the "pause on rate limit" logic. Because we already stop when we receive other http error responses, we can simply use this logic instead.
Note: feat() over ref() because it's a behaviour change, not because handling rate limits is entirely new (#6710)
closes#6984
@Lms24
Lms24
requested review from
billyvg and mydeaFebruary 1, 2023 11:18
@Lms24
Lms24force-pushed
the
lms-replay-stop-on-ratelimit
branch
from
d074416 to
4c2a170CompareFebruary 1, 2023 11:21
Lms24
Lms24
commented
Feb 1, 2023
View reviewed changes
Comment threadpackages/replay/test/integration/rateLimiting.test.ts
 replay && replay.stop();
 });

 it.each([
@Lms24Lms24Feb 1, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

IMO we don't need these tests anymore, as they only tested that we pause correctly for different rate limit timeouts we might receive from the Sentry backend. Just ensuring that we don't do anything after a 429 response should be good enough
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yes, makes sense! 👍 We could even move it to the general sendReplay tests I guess?
@Lms24Lms24Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Generally I think it makes sense but given that we have this neat comment under the last test,
// NOTE: If you add a test after the last one, make sure to adjust the test setup// As this ends with a `stopped()` replay, which may prevent future tests from working// Sadly, fixing this turned out to be much more annoying than expected, so leaving this warning here for now
and that I ran exactly into this problem, wasting 30minutes trying fix it, I'd vote we leave it as is.
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ahh, right 🙈 damn we should fix those xD
@Lms24Lms24
changed the title
rfe(replay): Stop recording when hitting a rate limitref(replay): Stop recording when hitting a rate limitFeb 1, 2023
@github-actions
github-actionsBot
commented
Feb 1, 2023

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.88 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.54 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.29 KB (0%)
@sentry/browser - Webpack (minified)66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.08 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)43.79 KB (-0.85% 🔽)
@sentry/replay - Webpack (gzipped + minified)38.7 KB (-0.5% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)61.38 KB (-0.21% 🔽)
mydea
mydea
approved these changes
Feb 1, 2023
View reviewed changes
@mydeamydea
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nice, saving some bytes!
l: We may want to move the tests to the general send replay tests, I guess we don't need a separate test module then. but this is super unimportant. Thanks for tackling this! 👍
@mydea
mydea
commented
Feb 1, 2023

Copy link


Copy Markdown

Member
One comment: I guess this should be feat, not ref, as it kind of changes the behavior? WDYT?
@Lms24Lms24
changed the title
ref(replay): Stop recording when hitting a rate limitfeat(replay): Stop recording when hitting a rate limitFeb 1, 2023
@Lms24
Lms24
merged commit 6ddc5cd
into
developFeb 1, 2023
@Lms24
Lms24
deleted the

lms-replay-stop-on-ratelimit

branch
February 1, 2023 16:25
@mydeamydea
mentioned this pull request
Oct 31, 2023
 Merged

mydea added a commit
that referenced
this pull request
Nov 2, 2023
@mydea
fix(replay): Ensure we stop for rate limit headers (#9420)
f796d50
We changed this
[here](#7018), but
apparently it is possible to have responses with a 200 status code but
rate limit headers.
This PR updates our handling to stop either for a non-200 status code,
_or_ for a rate limit header.
I also streamlined the tests for this a bit, we were testing a bunch of
unrelated things, IMHO it's enough to test we stopped/didn't stop.
Resolves: getsentry/sentry#49498
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
@billyvgbillyvgbillyvg approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Stop replay recording when hitting a rate limit


3 participants

@Lms24@mydea@billyvg

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
feat(replay): Stop recording when hitting a rate limit - #7018
Merged
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Feb 1, 2023
Merged
feat(replay): Stop recording when hitting a rate limit#7018
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Conversation
@Lms24
@Lms24Lms24
commented
Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

Member
As discussed and outlined in #6984, we want to entirely stop recording the replay once we receive a 429 rate limit response. This PR gets rid of all the "pause on rate limit" logic. Because we already stop when we receive other http error responses, we can simply use this logic instead.
Note: feat() over ref() because it's a behaviour change, not because handling rate limits is entirely new (#6710)
closes#6984
@Lms24
Lms24
requested review from
billyvg and mydeaFebruary 1, 2023 11:18
@Lms24
Lms24force-pushed
the
lms-replay-stop-on-ratelimit
branch
from
d074416 to
4c2a170CompareFebruary 1, 2023 11:21
Lms24
Lms24
commented
Feb 1, 2023
View reviewed changes
Comment threadpackages/replay/test/integration/rateLimiting.test.ts
 replay && replay.stop();
 });

 it.each([
@Lms24Lms24Feb 1, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

IMO we don't need these tests anymore, as they only tested that we pause correctly for different rate limit timeouts we might receive from the Sentry backend. Just ensuring that we don't do anything after a 429 response should be good enough
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yes, makes sense! 👍 We could even move it to the general sendReplay tests I guess?
@Lms24Lms24Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Generally I think it makes sense but given that we have this neat comment under the last test,
// NOTE: If you add a test after the last one, make sure to adjust the test setup// As this ends with a `stopped()` replay, which may prevent future tests from working// Sadly, fixing this turned out to be much more annoying than expected, so leaving this warning here for now
and that I ran exactly into this problem, wasting 30minutes trying fix it, I'd vote we leave it as is.
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ahh, right 🙈 damn we should fix those xD
@Lms24Lms24
changed the title
rfe(replay): Stop recording when hitting a rate limitref(replay): Stop recording when hitting a rate limitFeb 1, 2023
@github-actions
github-actionsBot
commented
Feb 1, 2023

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.88 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.54 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.29 KB (0%)
@sentry/browser - Webpack (minified)66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.08 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)43.79 KB (-0.85% 🔽)
@sentry/replay - Webpack (gzipped + minified)38.7 KB (-0.5% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)61.38 KB (-0.21% 🔽)
mydea
mydea
approved these changes
Feb 1, 2023
View reviewed changes
@mydeamydea
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nice, saving some bytes!
l: We may want to move the tests to the general send replay tests, I guess we don't need a separate test module then. but this is super unimportant. Thanks for tackling this! 👍
@mydea
mydea
commented
Feb 1, 2023

Copy link


Copy Markdown

Member
One comment: I guess this should be feat, not ref, as it kind of changes the behavior? WDYT?
@Lms24Lms24
changed the title
ref(replay): Stop recording when hitting a rate limitfeat(replay): Stop recording when hitting a rate limitFeb 1, 2023
@Lms24
Lms24
merged commit 6ddc5cd
into
developFeb 1, 2023
@Lms24
Lms24
deleted the

lms-replay-stop-on-ratelimit

branch
February 1, 2023 16:25
@mydeamydea
mentioned this pull request
Oct 31, 2023
 Merged

mydea added a commit
that referenced
this pull request
Nov 2, 2023
@mydea
fix(replay): Ensure we stop for rate limit headers (#9420)
f796d50
We changed this
[here](#7018), but
apparently it is possible to have responses with a 200 status code but
rate limit headers.
This PR updates our handling to stop either for a non-200 status code,
_or_ for a rate limit header.
I also streamlined the tests for this a bit, we were testing a bunch of
unrelated things, IMHO it's enough to test we stopped/didn't stop.
Resolves: getsentry/sentry#49498
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
@billyvgbillyvgbillyvg approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Stop replay recording when hitting a rate limit


3 participants

@Lms24@mydea@billyvg

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(replay): Stop recording when hitting a rate limit - #7018
Merged
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Feb 1, 2023
Merged
feat(replay): Stop recording when hitting a rate limit#7018
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Conversation
@Lms24
@Lms24Lms24
commented
Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

Member
As discussed and outlined in #6984, we want to entirely stop recording the replay once we receive a 429 rate limit response. This PR gets rid of all the "pause on rate limit" logic. Because we already stop when we receive other http error responses, we can simply use this logic instead.
Note: feat() over ref() because it's a behaviour change, not because handling rate limits is entirely new (#6710)
closes#6984
@Lms24
Lms24
requested review from
billyvg and mydeaFebruary 1, 2023 11:18
@Lms24
Lms24force-pushed
the
lms-replay-stop-on-ratelimit
branch
from
d074416 to
4c2a170CompareFebruary 1, 2023 11:21
Lms24
Lms24
commented
Feb 1, 2023
View reviewed changes
Comment threadpackages/replay/test/integration/rateLimiting.test.ts
 replay && replay.stop();
 });

 it.each([
@Lms24Lms24Feb 1, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

IMO we don't need these tests anymore, as they only tested that we pause correctly for different rate limit timeouts we might receive from the Sentry backend. Just ensuring that we don't do anything after a 429 response should be good enough
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yes, makes sense! 👍 We could even move it to the general sendReplay tests I guess?
@Lms24Lms24Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Generally I think it makes sense but given that we have this neat comment under the last test,
// NOTE: If you add a test after the last one, make sure to adjust the test setup// As this ends with a `stopped()` replay, which may prevent future tests from working// Sadly, fixing this turned out to be much more annoying than expected, so leaving this warning here for now
and that I ran exactly into this problem, wasting 30minutes trying fix it, I'd vote we leave it as is.
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ahh, right 🙈 damn we should fix those xD
@Lms24Lms24
changed the title
rfe(replay): Stop recording when hitting a rate limitref(replay): Stop recording when hitting a rate limitFeb 1, 2023
@github-actions
github-actionsBot
commented
Feb 1, 2023

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.88 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.54 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.29 KB (0%)
@sentry/browser - Webpack (minified)66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.08 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)43.79 KB (-0.85% 🔽)
@sentry/replay - Webpack (gzipped + minified)38.7 KB (-0.5% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)61.38 KB (-0.21% 🔽)
mydea
mydea
approved these changes
Feb 1, 2023
View reviewed changes
@mydeamydea
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nice, saving some bytes!
l: We may want to move the tests to the general send replay tests, I guess we don't need a separate test module then. but this is super unimportant. Thanks for tackling this! 👍
@mydea
mydea
commented
Feb 1, 2023

Copy link


Copy Markdown

Member
One comment: I guess this should be feat, not ref, as it kind of changes the behavior? WDYT?
@Lms24Lms24
changed the title
ref(replay): Stop recording when hitting a rate limitfeat(replay): Stop recording when hitting a rate limitFeb 1, 2023
@Lms24
Lms24
merged commit 6ddc5cd
into
developFeb 1, 2023
@Lms24
Lms24
deleted the

lms-replay-stop-on-ratelimit

branch
February 1, 2023 16:25
@mydeamydea
mentioned this pull request
Oct 31, 2023
 Merged

mydea added a commit
that referenced
this pull request
Nov 2, 2023
@mydea
fix(replay): Ensure we stop for rate limit headers (#9420)
f796d50
We changed this
[here](#7018), but
apparently it is possible to have responses with a 200 status code but
rate limit headers.
This PR updates our handling to stop either for a non-200 status code,
_or_ for a rate limit header.
I also streamlined the tests for this a bit, we were testing a bunch of
unrelated things, IMHO it's enough to test we stopped/didn't stop.
Resolves: getsentry/sentry#49498
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
@billyvgbillyvgbillyvg approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Stop replay recording when hitting a rate limit


3 participants

@Lms24@mydea@billyvg

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(replay): Stop recording when hitting a rate limit - #7018
Merged
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Feb 1, 2023
Merged
feat(replay): Stop recording when hitting a rate limit#7018
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Conversation
@Lms24
@Lms24Lms24
commented
Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

Member
As discussed and outlined in #6984, we want to entirely stop recording the replay once we receive a 429 rate limit response. This PR gets rid of all the "pause on rate limit" logic. Because we already stop when we receive other http error responses, we can simply use this logic instead.
Note: feat() over ref() because it's a behaviour change, not because handling rate limits is entirely new (#6710)
closes#6984
@Lms24
Lms24
requested review from
billyvg and mydeaFebruary 1, 2023 11:18
@Lms24
Lms24force-pushed
the
lms-replay-stop-on-ratelimit
branch
from
d074416 to
4c2a170CompareFebruary 1, 2023 11:21
Lms24
Lms24
commented
Feb 1, 2023
View reviewed changes
Comment threadpackages/replay/test/integration/rateLimiting.test.ts
 replay && replay.stop();
 });

 it.each([
@Lms24Lms24Feb 1, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

IMO we don't need these tests anymore, as they only tested that we pause correctly for different rate limit timeouts we might receive from the Sentry backend. Just ensuring that we don't do anything after a 429 response should be good enough
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yes, makes sense! 👍 We could even move it to the general sendReplay tests I guess?
@Lms24Lms24Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Generally I think it makes sense but given that we have this neat comment under the last test,
// NOTE: If you add a test after the last one, make sure to adjust the test setup// As this ends with a `stopped()` replay, which may prevent future tests from working// Sadly, fixing this turned out to be much more annoying than expected, so leaving this warning here for now
and that I ran exactly into this problem, wasting 30minutes trying fix it, I'd vote we leave it as is.
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ahh, right 🙈 damn we should fix those xD
@Lms24Lms24
changed the title
rfe(replay): Stop recording when hitting a rate limitref(replay): Stop recording when hitting a rate limitFeb 1, 2023
@github-actions
github-actionsBot
commented
Feb 1, 2023

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.88 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.54 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.29 KB (0%)
@sentry/browser - Webpack (minified)66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.08 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)43.79 KB (-0.85% 🔽)
@sentry/replay - Webpack (gzipped + minified)38.7 KB (-0.5% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)61.38 KB (-0.21% 🔽)
mydea
mydea
approved these changes
Feb 1, 2023
View reviewed changes
@mydeamydea
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nice, saving some bytes!
l: We may want to move the tests to the general send replay tests, I guess we don't need a separate test module then. but this is super unimportant. Thanks for tackling this! 👍
@mydea
mydea
commented
Feb 1, 2023

Copy link


Copy Markdown

Member
One comment: I guess this should be feat, not ref, as it kind of changes the behavior? WDYT?
@Lms24Lms24
changed the title
ref(replay): Stop recording when hitting a rate limitfeat(replay): Stop recording when hitting a rate limitFeb 1, 2023
@Lms24
Lms24
merged commit 6ddc5cd
into
developFeb 1, 2023
@Lms24
Lms24
deleted the

lms-replay-stop-on-ratelimit

branch
February 1, 2023 16:25
@mydeamydea
mentioned this pull request
Oct 31, 2023
 Merged

mydea added a commit
that referenced
this pull request
Nov 2, 2023
@mydea
fix(replay): Ensure we stop for rate limit headers (#9420)
f796d50
We changed this
[here](#7018), but
apparently it is possible to have responses with a 200 status code but
rate limit headers.
This PR updates our handling to stop either for a non-200 status code,
_or_ for a rate limit header.
I also streamlined the tests for this a bit, we were testing a bunch of
unrelated things, IMHO it's enough to test we stopped/didn't stop.
Resolves: getsentry/sentry#49498
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
@billyvgbillyvgbillyvg approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Stop replay recording when hitting a rate limit


3 participants

@Lms24@mydea@billyvg

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
feat(replay): Stop recording when hitting a rate limit - #7018
Merged
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Feb 1, 2023
Merged
feat(replay): Stop recording when hitting a rate limit#7018
Lms24 merged 2 commits into
developfrom 
lms-replay-stop-on-ratelimit
Conversation
@Lms24
@Lms24Lms24
commented
Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

Member
As discussed and outlined in #6984, we want to entirely stop recording the replay once we receive a 429 rate limit response. This PR gets rid of all the "pause on rate limit" logic. Because we already stop when we receive other http error responses, we can simply use this logic instead.
Note: feat() over ref() because it's a behaviour change, not because handling rate limits is entirely new (#6710)
closes#6984
@Lms24
Lms24
requested review from
billyvg and mydeaFebruary 1, 2023 11:18
@Lms24
Lms24force-pushed
the
lms-replay-stop-on-ratelimit
branch
from
d074416 to
4c2a170CompareFebruary 1, 2023 11:21
Lms24
Lms24
commented
Feb 1, 2023
View reviewed changes
Comment threadpackages/replay/test/integration/rateLimiting.test.ts
 replay && replay.stop();
 });

 it.each([
@Lms24Lms24Feb 1, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

IMO we don't need these tests anymore, as they only tested that we pause correctly for different rate limit timeouts we might receive from the Sentry backend. Just ensuring that we don't do anything after a 429 response should be good enough
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Yes, makes sense! 👍 We could even move it to the general sendReplay tests I guess?
@Lms24Lms24Feb 1, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Generally I think it makes sense but given that we have this neat comment under the last test,
// NOTE: If you add a test after the last one, make sure to adjust the test setup// As this ends with a `stopped()` replay, which may prevent future tests from working// Sadly, fixing this turned out to be much more annoying than expected, so leaving this warning here for now
and that I ran exactly into this problem, wasting 30minutes trying fix it, I'd vote we leave it as is.
@mydeamydeaFeb 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

ahh, right 🙈 damn we should fix those xD
@Lms24Lms24
changed the title
rfe(replay): Stop recording when hitting a rate limitref(replay): Stop recording when hitting a rate limitFeb 1, 2023
@github-actions
github-actionsBot
commented
Feb 1, 2023

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize
@sentry/browser - ES5 CDN Bundle (gzipped + minified)19.88 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)61.57 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)18.54 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)54.88 KB (0%)
@sentry/browser - Webpack (gzipped + minified)20.29 KB (0%)
@sentry/browser - Webpack (minified)66.37 KB (0%)
@sentry/react - Webpack (gzipped + minified)20.31 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)47.6 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)26.79 KB (0%)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)25.08 KB (0%)
@sentry/replay ES6 CDN Bundle (gzipped + minified)43.79 KB (-0.85% 🔽)
@sentry/replay - Webpack (gzipped + minified)38.7 KB (-0.5% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)61.38 KB (-0.21% 🔽)
mydea
mydea
approved these changes
Feb 1, 2023
View reviewed changes
@mydeamydea
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Nice, saving some bytes!
l: We may want to move the tests to the general send replay tests, I guess we don't need a separate test module then. but this is super unimportant. Thanks for tackling this! 👍
@mydea
mydea
commented
Feb 1, 2023

Copy link


Copy Markdown

Member
One comment: I guess this should be feat, not ref, as it kind of changes the behavior? WDYT?
@Lms24Lms24
changed the title
ref(replay): Stop recording when hitting a rate limitfeat(replay): Stop recording when hitting a rate limitFeb 1, 2023
@Lms24
Lms24
merged commit 6ddc5cd
into
developFeb 1, 2023
@Lms24
Lms24
deleted the

lms-replay-stop-on-ratelimit

branch
February 1, 2023 16:25
@mydeamydea
mentioned this pull request
Oct 31, 2023
 Merged

mydea added a commit
that referenced
this pull request
Nov 2, 2023
@mydea
fix(replay): Ensure we stop for rate limit headers (#9420)
f796d50
We changed this
[here](#7018), but
apparently it is possible to have responses with a 200 status code but
rate limit headers.
This PR updates our handling to stop either for a non-200 status code,
_or_ for a rate limit header.
I also streamlined the tests for this a bit, we were testing a bunch of
unrelated things, IMHO it's enough to test we stopped/didn't stop.
Resolves: getsentry/sentry#49498
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@mydeamydeamydea approved these changes
@billyvgbillyvgbillyvg approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Stop replay recording when hitting a rate limit


3 participants

@Lms24@mydea@billyvg