fix(profiling): Classify profile chunks for rate limits and outcomes - #4595

Merged
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks
Mar 21, 2025
Merged

fix(profiling): Classify profile chunks for rate limits and outcomes#4595
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks

Conversation

@Dav1dde

@Dav1ddeDav1dde commented Mar 20, 2025

Copy link
Copy Markdown
Member

This separates profile chunks into two separate categories. The category is determined by the platform contained within the item payload.

This does have several downsides:

  • No outcomes are emitted until the platform can be determined.
  • Rate limits happen only in processing (this is the only code which currently extracts the platform).

The plan is to hoist the platform into an item header, provided by the SDK directly. Once this happened we can reject all profile chunks without that necessary platform item and we can start rate limiting in the fast path.

Additionally we'll change Relay to make the executive decision whether a profile chunk is frontend/backend, this means we no longer need to make that decision in multiple places (they cannot diverge).

We will also need to consider how often the platforms change, we can extend Relay in a way where unknown platforms are just forwarded to processing but for the sake of rate limiting considered as the default category. Or just make the assignment configurable via global config.

The test test_profile_chunk_outcomes_rate_limited_via_profile_duration_rate_limit was never correct, the duration quota never worked it always just worked because it also contained the chunk category, but that is equivalent to the test above.

Fixes: https://github.com/getsentry/team-ingest/issues/679

@Dav1dde
Dav1dde requested a review from a team as a code ownerMarch 20, 2025 14:36
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch 3 times, most recently from f6de8a0 to 65c2067CompareMarch 20, 2025 17:13
@Dav1ddeDav1dde self-assigned this Mar 20, 2025
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from 65c2067 to d53c559CompareMarch 20, 2025 17:35
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from d53c559 to 4df7283CompareMarch 20, 2025 17:36

@loewenheimloewenheim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typo, looks good otherwise

Comment threadrelay-server/src/utils/rate_limits.rs Outdated
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
Comment on lines +315 to 320
pub fn profile_type(&self) -> ProfileType {
match self.profile.platform.as_str() {
"cocoa" | "android" | "javascript" => ProfileType::Ui,
_ => ProfileType::Backend,
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

/// The profile type is currently determined based on the contained profile
/// platform. It determines the data category this profile chunk belongs to.
///
/// This needs to be synchronized with the implementation in Sentry:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can add the profile_type in the payload before we send it, this would become the source of truth.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use add it to the Kafka message https://github.com/getsentry/relay/blob/master/relay-server/src/services/store.rs#L1035-L1041 and the worker will handle it, no need to deserialize/serialize again.

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.

Sure, that works, let's figure that out in a follow up where we wanna put it (payload / kafka header) and how it should look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add it to the payload and not the header, it's easier to manipulate later on.

ItemType::ProfileChunk => match item.profile_type() {
Some(ProfileType::Backend) => !self.profile_chunks.is_active(),
Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(),
None => true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this reject or accept profiles when we don't have a type?

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.

It will accept when there is no type.

Comment threadtests/integration/test_filters.py Outdated
@phacops

Copy link
Copy Markdown
Contributor

We started adding a platform field to SDKs, Relay should then classify the chunk based on this, add the type to the payload before we push to Kafka and become the source of truth.

We will also need to consider how often the platforms change

Platforms are not added very often, UI platforms even less often and rejecting the chunk for unknown/empty platforms is a good default.

@Dav1dde
Dav1dde enabled auto-merge (squash) March 21, 2025 08:44
@Dav1dde
Dav1dde merged commit cd67f90 into masterMar 21, 2025
@Dav1dde
Dav1dde deleted the dav1d/ratelimit-profile-chunks branch March 21, 2025 08:47
Sign up for freeto 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

@Dav1dde@phacops@olksdr@loewenheim@Litarnus
, '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

fix(profiling): Classify profile chunks for rate limits and outcomes - #4595

Merged
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks
Mar 21, 2025
Merged

fix(profiling): Classify profile chunks for rate limits and outcomes#4595
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks

Conversation

@Dav1dde

@Dav1ddeDav1dde commented Mar 20, 2025

Copy link
Copy Markdown
Member

This separates profile chunks into two separate categories. The category is determined by the platform contained within the item payload.

This does have several downsides:

  • No outcomes are emitted until the platform can be determined.
  • Rate limits happen only in processing (this is the only code which currently extracts the platform).

The plan is to hoist the platform into an item header, provided by the SDK directly. Once this happened we can reject all profile chunks without that necessary platform item and we can start rate limiting in the fast path.

Additionally we'll change Relay to make the executive decision whether a profile chunk is frontend/backend, this means we no longer need to make that decision in multiple places (they cannot diverge).

We will also need to consider how often the platforms change, we can extend Relay in a way where unknown platforms are just forwarded to processing but for the sake of rate limiting considered as the default category. Or just make the assignment configurable via global config.

The test test_profile_chunk_outcomes_rate_limited_via_profile_duration_rate_limit was never correct, the duration quota never worked it always just worked because it also contained the chunk category, but that is equivalent to the test above.

Fixes: https://github.com/getsentry/team-ingest/issues/679

@Dav1dde
Dav1dde requested a review from a team as a code ownerMarch 20, 2025 14:36
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch 3 times, most recently from f6de8a0 to 65c2067CompareMarch 20, 2025 17:13
@Dav1ddeDav1dde self-assigned this Mar 20, 2025
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from 65c2067 to d53c559CompareMarch 20, 2025 17:35
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from d53c559 to 4df7283CompareMarch 20, 2025 17:36

@loewenheimloewenheim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typo, looks good otherwise

Comment threadrelay-server/src/utils/rate_limits.rs Outdated
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
Comment on lines +315 to 320
pub fn profile_type(&self) -> ProfileType {
match self.profile.platform.as_str() {
"cocoa" | "android" | "javascript" => ProfileType::Ui,
_ => ProfileType::Backend,
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

/// The profile type is currently determined based on the contained profile
/// platform. It determines the data category this profile chunk belongs to.
///
/// This needs to be synchronized with the implementation in Sentry:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can add the profile_type in the payload before we send it, this would become the source of truth.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use add it to the Kafka message https://github.com/getsentry/relay/blob/master/relay-server/src/services/store.rs#L1035-L1041 and the worker will handle it, no need to deserialize/serialize again.

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.

Sure, that works, let's figure that out in a follow up where we wanna put it (payload / kafka header) and how it should look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add it to the payload and not the header, it's easier to manipulate later on.

ItemType::ProfileChunk => match item.profile_type() {
Some(ProfileType::Backend) => !self.profile_chunks.is_active(),
Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(),
None => true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this reject or accept profiles when we don't have a type?

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.

It will accept when there is no type.

Comment threadtests/integration/test_filters.py Outdated
@phacops

Copy link
Copy Markdown
Contributor

We started adding a platform field to SDKs, Relay should then classify the chunk based on this, add the type to the payload before we push to Kafka and become the source of truth.

We will also need to consider how often the platforms change

Platforms are not added very often, UI platforms even less often and rejecting the chunk for unknown/empty platforms is a good default.

@Dav1dde
Dav1dde enabled auto-merge (squash) March 21, 2025 08:44
@Dav1dde
Dav1dde merged commit cd67f90 into masterMar 21, 2025
@Dav1dde
Dav1dde deleted the dav1d/ratelimit-profile-chunks branch March 21, 2025 08:47
Sign up for freeto 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

@Dav1dde@phacops@olksdr@loewenheim@Litarnus
, '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

fix(profiling): Classify profile chunks for rate limits and outcomes - #4595

Merged
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks
Mar 21, 2025
Merged

fix(profiling): Classify profile chunks for rate limits and outcomes#4595
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks

Conversation

@Dav1dde

@Dav1ddeDav1dde commented Mar 20, 2025

Copy link
Copy Markdown
Member

This separates profile chunks into two separate categories. The category is determined by the platform contained within the item payload.

This does have several downsides:

  • No outcomes are emitted until the platform can be determined.
  • Rate limits happen only in processing (this is the only code which currently extracts the platform).

The plan is to hoist the platform into an item header, provided by the SDK directly. Once this happened we can reject all profile chunks without that necessary platform item and we can start rate limiting in the fast path.

Additionally we'll change Relay to make the executive decision whether a profile chunk is frontend/backend, this means we no longer need to make that decision in multiple places (they cannot diverge).

We will also need to consider how often the platforms change, we can extend Relay in a way where unknown platforms are just forwarded to processing but for the sake of rate limiting considered as the default category. Or just make the assignment configurable via global config.

The test test_profile_chunk_outcomes_rate_limited_via_profile_duration_rate_limit was never correct, the duration quota never worked it always just worked because it also contained the chunk category, but that is equivalent to the test above.

Fixes: https://github.com/getsentry/team-ingest/issues/679

@Dav1dde
Dav1dde requested a review from a team as a code ownerMarch 20, 2025 14:36
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch 3 times, most recently from f6de8a0 to 65c2067CompareMarch 20, 2025 17:13
@Dav1ddeDav1dde self-assigned this Mar 20, 2025
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from 65c2067 to d53c559CompareMarch 20, 2025 17:35
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from d53c559 to 4df7283CompareMarch 20, 2025 17:36

@loewenheimloewenheim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typo, looks good otherwise

Comment threadrelay-server/src/utils/rate_limits.rs Outdated
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
Comment on lines +315 to 320
pub fn profile_type(&self) -> ProfileType {
match self.profile.platform.as_str() {
"cocoa" | "android" | "javascript" => ProfileType::Ui,
_ => ProfileType::Backend,
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

/// The profile type is currently determined based on the contained profile
/// platform. It determines the data category this profile chunk belongs to.
///
/// This needs to be synchronized with the implementation in Sentry:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can add the profile_type in the payload before we send it, this would become the source of truth.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use add it to the Kafka message https://github.com/getsentry/relay/blob/master/relay-server/src/services/store.rs#L1035-L1041 and the worker will handle it, no need to deserialize/serialize again.

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.

Sure, that works, let's figure that out in a follow up where we wanna put it (payload / kafka header) and how it should look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add it to the payload and not the header, it's easier to manipulate later on.

ItemType::ProfileChunk => match item.profile_type() {
Some(ProfileType::Backend) => !self.profile_chunks.is_active(),
Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(),
None => true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this reject or accept profiles when we don't have a type?

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.

It will accept when there is no type.

Comment threadtests/integration/test_filters.py Outdated
@phacops

Copy link
Copy Markdown
Contributor

We started adding a platform field to SDKs, Relay should then classify the chunk based on this, add the type to the payload before we push to Kafka and become the source of truth.

We will also need to consider how often the platforms change

Platforms are not added very often, UI platforms even less often and rejecting the chunk for unknown/empty platforms is a good default.

@Dav1dde
Dav1dde enabled auto-merge (squash) March 21, 2025 08:44
@Dav1dde
Dav1dde merged commit cd67f90 into masterMar 21, 2025
@Dav1dde
Dav1dde deleted the dav1d/ratelimit-profile-chunks branch March 21, 2025 08:47
Sign up for freeto 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

@Dav1dde@phacops@olksdr@loewenheim@Litarnus
, '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

fix(profiling): Classify profile chunks for rate limits and outcomes - #4595

Merged
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks
Mar 21, 2025
Merged

fix(profiling): Classify profile chunks for rate limits and outcomes#4595
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks

Conversation

@Dav1dde

@Dav1ddeDav1dde commented Mar 20, 2025

Copy link
Copy Markdown
Member

This separates profile chunks into two separate categories. The category is determined by the platform contained within the item payload.

This does have several downsides:

  • No outcomes are emitted until the platform can be determined.
  • Rate limits happen only in processing (this is the only code which currently extracts the platform).

The plan is to hoist the platform into an item header, provided by the SDK directly. Once this happened we can reject all profile chunks without that necessary platform item and we can start rate limiting in the fast path.

Additionally we'll change Relay to make the executive decision whether a profile chunk is frontend/backend, this means we no longer need to make that decision in multiple places (they cannot diverge).

We will also need to consider how often the platforms change, we can extend Relay in a way where unknown platforms are just forwarded to processing but for the sake of rate limiting considered as the default category. Or just make the assignment configurable via global config.

The test test_profile_chunk_outcomes_rate_limited_via_profile_duration_rate_limit was never correct, the duration quota never worked it always just worked because it also contained the chunk category, but that is equivalent to the test above.

Fixes: https://github.com/getsentry/team-ingest/issues/679

@Dav1dde
Dav1dde requested a review from a team as a code ownerMarch 20, 2025 14:36
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch 3 times, most recently from f6de8a0 to 65c2067CompareMarch 20, 2025 17:13
@Dav1ddeDav1dde self-assigned this Mar 20, 2025
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from 65c2067 to d53c559CompareMarch 20, 2025 17:35
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from d53c559 to 4df7283CompareMarch 20, 2025 17:36

@loewenheimloewenheim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typo, looks good otherwise

Comment threadrelay-server/src/utils/rate_limits.rs Outdated
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
Comment on lines +315 to 320
pub fn profile_type(&self) -> ProfileType {
match self.profile.platform.as_str() {
"cocoa" | "android" | "javascript" => ProfileType::Ui,
_ => ProfileType::Backend,
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

/// The profile type is currently determined based on the contained profile
/// platform. It determines the data category this profile chunk belongs to.
///
/// This needs to be synchronized with the implementation in Sentry:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can add the profile_type in the payload before we send it, this would become the source of truth.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use add it to the Kafka message https://github.com/getsentry/relay/blob/master/relay-server/src/services/store.rs#L1035-L1041 and the worker will handle it, no need to deserialize/serialize again.

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.

Sure, that works, let's figure that out in a follow up where we wanna put it (payload / kafka header) and how it should look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add it to the payload and not the header, it's easier to manipulate later on.

ItemType::ProfileChunk => match item.profile_type() {
Some(ProfileType::Backend) => !self.profile_chunks.is_active(),
Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(),
None => true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this reject or accept profiles when we don't have a type?

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.

It will accept when there is no type.

Comment threadtests/integration/test_filters.py Outdated
@phacops

Copy link
Copy Markdown
Contributor

We started adding a platform field to SDKs, Relay should then classify the chunk based on this, add the type to the payload before we push to Kafka and become the source of truth.

We will also need to consider how often the platforms change

Platforms are not added very often, UI platforms even less often and rejecting the chunk for unknown/empty platforms is a good default.

@Dav1dde
Dav1dde enabled auto-merge (squash) March 21, 2025 08:44
@Dav1dde
Dav1dde merged commit cd67f90 into masterMar 21, 2025
@Dav1dde
Dav1dde deleted the dav1d/ratelimit-profile-chunks branch March 21, 2025 08:47
Sign up for freeto 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

@Dav1dde@phacops@olksdr@loewenheim@Litarnus
, '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

fix(profiling): Classify profile chunks for rate limits and outcomes - #4595

Merged
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks
Mar 21, 2025
Merged

fix(profiling): Classify profile chunks for rate limits and outcomes#4595
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks

Conversation

@Dav1dde

@Dav1ddeDav1dde commented Mar 20, 2025

Copy link
Copy Markdown
Member

This separates profile chunks into two separate categories. The category is determined by the platform contained within the item payload.

This does have several downsides:

  • No outcomes are emitted until the platform can be determined.
  • Rate limits happen only in processing (this is the only code which currently extracts the platform).

The plan is to hoist the platform into an item header, provided by the SDK directly. Once this happened we can reject all profile chunks without that necessary platform item and we can start rate limiting in the fast path.

Additionally we'll change Relay to make the executive decision whether a profile chunk is frontend/backend, this means we no longer need to make that decision in multiple places (they cannot diverge).

We will also need to consider how often the platforms change, we can extend Relay in a way where unknown platforms are just forwarded to processing but for the sake of rate limiting considered as the default category. Or just make the assignment configurable via global config.

The test test_profile_chunk_outcomes_rate_limited_via_profile_duration_rate_limit was never correct, the duration quota never worked it always just worked because it also contained the chunk category, but that is equivalent to the test above.

Fixes: https://github.com/getsentry/team-ingest/issues/679

@Dav1dde
Dav1dde requested a review from a team as a code ownerMarch 20, 2025 14:36
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch 3 times, most recently from f6de8a0 to 65c2067CompareMarch 20, 2025 17:13
@Dav1ddeDav1dde self-assigned this Mar 20, 2025
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from 65c2067 to d53c559CompareMarch 20, 2025 17:35
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from d53c559 to 4df7283CompareMarch 20, 2025 17:36

@loewenheimloewenheim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typo, looks good otherwise

Comment threadrelay-server/src/utils/rate_limits.rs Outdated
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
Comment on lines +315 to 320
pub fn profile_type(&self) -> ProfileType {
match self.profile.platform.as_str() {
"cocoa" | "android" | "javascript" => ProfileType::Ui,
_ => ProfileType::Backend,
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

/// The profile type is currently determined based on the contained profile
/// platform. It determines the data category this profile chunk belongs to.
///
/// This needs to be synchronized with the implementation in Sentry:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can add the profile_type in the payload before we send it, this would become the source of truth.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use add it to the Kafka message https://github.com/getsentry/relay/blob/master/relay-server/src/services/store.rs#L1035-L1041 and the worker will handle it, no need to deserialize/serialize again.

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.

Sure, that works, let's figure that out in a follow up where we wanna put it (payload / kafka header) and how it should look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add it to the payload and not the header, it's easier to manipulate later on.

ItemType::ProfileChunk => match item.profile_type() {
Some(ProfileType::Backend) => !self.profile_chunks.is_active(),
Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(),
None => true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this reject or accept profiles when we don't have a type?

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.

It will accept when there is no type.

Comment threadtests/integration/test_filters.py Outdated
@phacops

Copy link
Copy Markdown
Contributor

We started adding a platform field to SDKs, Relay should then classify the chunk based on this, add the type to the payload before we push to Kafka and become the source of truth.

We will also need to consider how often the platforms change

Platforms are not added very often, UI platforms even less often and rejecting the chunk for unknown/empty platforms is a good default.

@Dav1dde
Dav1dde enabled auto-merge (squash) March 21, 2025 08:44
@Dav1dde
Dav1dde merged commit cd67f90 into masterMar 21, 2025
@Dav1dde
Dav1dde deleted the dav1d/ratelimit-profile-chunks branch March 21, 2025 08:47
Sign up for freeto 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

@Dav1dde@phacops@olksdr@loewenheim@Litarnus
, '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

fix(profiling): Classify profile chunks for rate limits and outcomes - #4595

Merged
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks
Mar 21, 2025
Merged

fix(profiling): Classify profile chunks for rate limits and outcomes#4595
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks

Conversation

@Dav1dde

@Dav1ddeDav1dde commented Mar 20, 2025

Copy link
Copy Markdown
Member

This separates profile chunks into two separate categories. The category is determined by the platform contained within the item payload.

This does have several downsides:

  • No outcomes are emitted until the platform can be determined.
  • Rate limits happen only in processing (this is the only code which currently extracts the platform).

The plan is to hoist the platform into an item header, provided by the SDK directly. Once this happened we can reject all profile chunks without that necessary platform item and we can start rate limiting in the fast path.

Additionally we'll change Relay to make the executive decision whether a profile chunk is frontend/backend, this means we no longer need to make that decision in multiple places (they cannot diverge).

We will also need to consider how often the platforms change, we can extend Relay in a way where unknown platforms are just forwarded to processing but for the sake of rate limiting considered as the default category. Or just make the assignment configurable via global config.

The test test_profile_chunk_outcomes_rate_limited_via_profile_duration_rate_limit was never correct, the duration quota never worked it always just worked because it also contained the chunk category, but that is equivalent to the test above.

Fixes: https://github.com/getsentry/team-ingest/issues/679

@Dav1dde
Dav1dde requested a review from a team as a code ownerMarch 20, 2025 14:36
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch 3 times, most recently from f6de8a0 to 65c2067CompareMarch 20, 2025 17:13
@Dav1ddeDav1dde self-assigned this Mar 20, 2025
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from 65c2067 to d53c559CompareMarch 20, 2025 17:35
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from d53c559 to 4df7283CompareMarch 20, 2025 17:36

@loewenheimloewenheim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typo, looks good otherwise

Comment threadrelay-server/src/utils/rate_limits.rs Outdated
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
Comment on lines +315 to 320
pub fn profile_type(&self) -> ProfileType {
match self.profile.platform.as_str() {
"cocoa" | "android" | "javascript" => ProfileType::Ui,
_ => ProfileType::Backend,
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

/// The profile type is currently determined based on the contained profile
/// platform. It determines the data category this profile chunk belongs to.
///
/// This needs to be synchronized with the implementation in Sentry:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can add the profile_type in the payload before we send it, this would become the source of truth.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use add it to the Kafka message https://github.com/getsentry/relay/blob/master/relay-server/src/services/store.rs#L1035-L1041 and the worker will handle it, no need to deserialize/serialize again.

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.

Sure, that works, let's figure that out in a follow up where we wanna put it (payload / kafka header) and how it should look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add it to the payload and not the header, it's easier to manipulate later on.

ItemType::ProfileChunk => match item.profile_type() {
Some(ProfileType::Backend) => !self.profile_chunks.is_active(),
Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(),
None => true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this reject or accept profiles when we don't have a type?

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.

It will accept when there is no type.

Comment threadtests/integration/test_filters.py Outdated
@phacops

Copy link
Copy Markdown
Contributor

We started adding a platform field to SDKs, Relay should then classify the chunk based on this, add the type to the payload before we push to Kafka and become the source of truth.

We will also need to consider how often the platforms change

Platforms are not added very often, UI platforms even less often and rejecting the chunk for unknown/empty platforms is a good default.

@Dav1dde
Dav1dde enabled auto-merge (squash) March 21, 2025 08:44
@Dav1dde
Dav1dde merged commit cd67f90 into masterMar 21, 2025
@Dav1dde
Dav1dde deleted the dav1d/ratelimit-profile-chunks branch March 21, 2025 08:47
Sign up for freeto 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

@Dav1dde@phacops@olksdr@loewenheim@Litarnus
, '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

fix(profiling): Classify profile chunks for rate limits and outcomes - #4595

Merged
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks
Mar 21, 2025
Merged

fix(profiling): Classify profile chunks for rate limits and outcomes#4595
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks

Conversation

@Dav1dde

@Dav1ddeDav1dde commented Mar 20, 2025

Copy link
Copy Markdown
Member

This separates profile chunks into two separate categories. The category is determined by the platform contained within the item payload.

This does have several downsides:

  • No outcomes are emitted until the platform can be determined.
  • Rate limits happen only in processing (this is the only code which currently extracts the platform).

The plan is to hoist the platform into an item header, provided by the SDK directly. Once this happened we can reject all profile chunks without that necessary platform item and we can start rate limiting in the fast path.

Additionally we'll change Relay to make the executive decision whether a profile chunk is frontend/backend, this means we no longer need to make that decision in multiple places (they cannot diverge).

We will also need to consider how often the platforms change, we can extend Relay in a way where unknown platforms are just forwarded to processing but for the sake of rate limiting considered as the default category. Or just make the assignment configurable via global config.

The test test_profile_chunk_outcomes_rate_limited_via_profile_duration_rate_limit was never correct, the duration quota never worked it always just worked because it also contained the chunk category, but that is equivalent to the test above.

Fixes: https://github.com/getsentry/team-ingest/issues/679

@Dav1dde
Dav1dde requested a review from a team as a code ownerMarch 20, 2025 14:36
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch 3 times, most recently from f6de8a0 to 65c2067CompareMarch 20, 2025 17:13
@Dav1ddeDav1dde self-assigned this Mar 20, 2025
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from 65c2067 to d53c559CompareMarch 20, 2025 17:35
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from d53c559 to 4df7283CompareMarch 20, 2025 17:36

@loewenheimloewenheim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typo, looks good otherwise

Comment threadrelay-server/src/utils/rate_limits.rs Outdated
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
Comment on lines +315 to 320
pub fn profile_type(&self) -> ProfileType {
match self.profile.platform.as_str() {
"cocoa" | "android" | "javascript" => ProfileType::Ui,
_ => ProfileType::Backend,
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

/// The profile type is currently determined based on the contained profile
/// platform. It determines the data category this profile chunk belongs to.
///
/// This needs to be synchronized with the implementation in Sentry:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can add the profile_type in the payload before we send it, this would become the source of truth.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use add it to the Kafka message https://github.com/getsentry/relay/blob/master/relay-server/src/services/store.rs#L1035-L1041 and the worker will handle it, no need to deserialize/serialize again.

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.

Sure, that works, let's figure that out in a follow up where we wanna put it (payload / kafka header) and how it should look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add it to the payload and not the header, it's easier to manipulate later on.

ItemType::ProfileChunk => match item.profile_type() {
Some(ProfileType::Backend) => !self.profile_chunks.is_active(),
Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(),
None => true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this reject or accept profiles when we don't have a type?

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.

It will accept when there is no type.

Comment threadtests/integration/test_filters.py Outdated
@phacops

Copy link
Copy Markdown
Contributor

We started adding a platform field to SDKs, Relay should then classify the chunk based on this, add the type to the payload before we push to Kafka and become the source of truth.

We will also need to consider how often the platforms change

Platforms are not added very often, UI platforms even less often and rejecting the chunk for unknown/empty platforms is a good default.

@Dav1dde
Dav1dde enabled auto-merge (squash) March 21, 2025 08:44
@Dav1dde
Dav1dde merged commit cd67f90 into masterMar 21, 2025
@Dav1dde
Dav1dde deleted the dav1d/ratelimit-profile-chunks branch March 21, 2025 08:47
Sign up for freeto 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

@Dav1dde@phacops@olksdr@loewenheim@Litarnus
, '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

fix(profiling): Classify profile chunks for rate limits and outcomes - #4595

Merged
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks
Mar 21, 2025
Merged

fix(profiling): Classify profile chunks for rate limits and outcomes#4595
Dav1dde merged 3 commits into
masterfrom
dav1d/ratelimit-profile-chunks

Conversation

@Dav1dde

@Dav1ddeDav1dde commented Mar 20, 2025

Copy link
Copy Markdown
Member

This separates profile chunks into two separate categories. The category is determined by the platform contained within the item payload.

This does have several downsides:

  • No outcomes are emitted until the platform can be determined.
  • Rate limits happen only in processing (this is the only code which currently extracts the platform).

The plan is to hoist the platform into an item header, provided by the SDK directly. Once this happened we can reject all profile chunks without that necessary platform item and we can start rate limiting in the fast path.

Additionally we'll change Relay to make the executive decision whether a profile chunk is frontend/backend, this means we no longer need to make that decision in multiple places (they cannot diverge).

We will also need to consider how often the platforms change, we can extend Relay in a way where unknown platforms are just forwarded to processing but for the sake of rate limiting considered as the default category. Or just make the assignment configurable via global config.

The test test_profile_chunk_outcomes_rate_limited_via_profile_duration_rate_limit was never correct, the duration quota never worked it always just worked because it also contained the chunk category, but that is equivalent to the test above.

Fixes: https://github.com/getsentry/team-ingest/issues/679

@Dav1dde
Dav1dde requested a review from a team as a code ownerMarch 20, 2025 14:36
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch 3 times, most recently from f6de8a0 to 65c2067CompareMarch 20, 2025 17:13
@Dav1ddeDav1dde self-assigned this Mar 20, 2025
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from 65c2067 to d53c559CompareMarch 20, 2025 17:35
@Dav1dde
Dav1ddeforce-pushed the dav1d/ratelimit-profile-chunks branch from d53c559 to 4df7283CompareMarch 20, 2025 17:36

@loewenheimloewenheim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small typo, looks good otherwise

Comment threadrelay-server/src/utils/rate_limits.rs Outdated
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
Comment on lines +315 to 320
pub fn profile_type(&self) -> ProfileType {
match self.profile.platform.as_str() {
"cocoa" | "android" | "javascript" => ProfileType::Ui,
_ => ProfileType::Backend,
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

/// The profile type is currently determined based on the contained profile
/// platform. It determines the data category this profile chunk belongs to.
///
/// This needs to be synchronized with the implementation in Sentry:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can add the profile_type in the payload before we send it, this would become the source of truth.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can use add it to the Kafka message https://github.com/getsentry/relay/blob/master/relay-server/src/services/store.rs#L1035-L1041 and the worker will handle it, no need to deserialize/serialize again.

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.

Sure, that works, let's figure that out in a follow up where we wanna put it (payload / kafka header) and how it should look?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would add it to the payload and not the header, it's easier to manipulate later on.

ItemType::ProfileChunk => match item.profile_type() {
Some(ProfileType::Backend) => !self.profile_chunks.is_active(),
Some(ProfileType::Ui) => !self.profile_chunks_ui.is_active(),
None => true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this reject or accept profiles when we don't have a type?

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.

It will accept when there is no type.

Comment threadtests/integration/test_filters.py Outdated
@phacops

Copy link
Copy Markdown
Contributor

We started adding a platform field to SDKs, Relay should then classify the chunk based on this, add the type to the payload before we push to Kafka and become the source of truth.

We will also need to consider how often the platforms change

Platforms are not added very often, UI platforms even less often and rejecting the chunk for unknown/empty platforms is a good default.

@Dav1dde
Dav1dde enabled auto-merge (squash) March 21, 2025 08:44
@Dav1dde
Dav1dde merged commit cd67f90 into masterMar 21, 2025
@Dav1dde
Dav1dde deleted the dav1d/ratelimit-profile-chunks branch March 21, 2025 08:47
Sign up for freeto 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

@Dav1dde@phacops@olksdr@loewenheim@Litarnus