Skip to content

feat(profiling): Route raw profiles through objectstore service#6025

Open
markushi wants to merge 5 commits into
feat/markushi/perfetto-profiling-support-pipelinefrom
feat/markushi/perfetto-profiling-support-pipeline-storage-mechanism
Open

feat(profiling): Route raw profiles through objectstore service#6025
markushi wants to merge 5 commits into
feat/markushi/perfetto-profiling-support-pipelinefrom
feat/markushi/perfetto-profiling-support-pipeline-storage-mechanism

Conversation

@markushi
Copy link
Copy Markdown
Member

@markushi markushi commented May 28, 2026

Summary

  • Extract StoreRawProfile into the objectstore service to upload raw profile blobs (e.g. Perfetto traces) before forwarding to Kafka
  • Replace inline raw profile blob in the Kafka message with an objectstore key reference
  • Objectstore service always forwards the StoreProfileChunk to the Store service, even if the upload fails, ensuring the Kafka message is produced

Test plan

  • Verify profile chunks without raw profiles are sent directly to Store
  • Verify profile chunks with raw profiles are routed through Objectstore
  • Verify Kafka message contains objectstore key reference instead of raw blob
  • Verify Kafka message is produced even when objectstore upload fails
  • Run integration tests: pytest tests/integration/test_profile_chunks_perfetto.py

🤖 Generated with Claude Code


This PR is part of a stack:

Extract StoreRawProfile into the objectstore service and replace
inline raw profile blob with objectstore key reference in the Kafka
message.

Co-Authored-By: Claude <noreply@anthropic.com>
@markushi markushi changed the base branch from master to feat/markushi/perfetto-profiling-support-pipeline May 28, 2026 07:51
Align the objectstore usecase identifier with the naming convention
so it can be easily distinguished from a future profiles_v2 usecase.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread relay-server/src/services/objectstore.rs
@markushi markushi marked this pull request as ready for review May 28, 2026 08:07
@markushi markushi requested a review from a team as a code owner May 28, 2026 08:07
Comment thread relay-server/src/processing/profile_chunks/mod.rs
Comment thread relay-server/src/processing/profile_chunks/mod.rs Outdated
Comment thread relay-server/src/services/objectstore.rs Outdated
Add try_send_to_objectstore to StoreHandle, which returns the message
when the objectstore service is not configured. Profile chunks fall
back to sending the StoreProfileChunk directly to Store, preventing
data loss. Also log session creation errors matching the pattern in
other objectstore handlers.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread relay-server/src/services/objectstore.rs Outdated
The rename in 3a83bc5 only updated MessageKind::as_str() but missed
the Usecase::new() call, causing raw profiles to be stored under the
generic "profiles" namespace instead of "profiles_raw".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread relay-server/src/services/objectstore.rs Outdated
let trace_attachments = Usecase::new("trace_attachments")
.with_expiration_policy(ExpirationPolicy::TimeToLive(DEFAULT_ATTACHMENT_RETENTION));
let profiles = Usecase::new("profiles")
.with_expiration_policy(ExpirationPolicy::TimeToLive(DEFAULT_ATTACHMENT_RETENTION));
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.

Is this what we want?

Copy link
Copy Markdown
Member

@lcian lcian May 28, 2026

Choose a reason for hiding this comment

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

Note that DEFAULT_ATTACHMENT_RETENTION means event attachments here. Even if that happens to be the correct retention you actually want, you likely want to use a different constant here to not mix things up.
Here you likely want to use the profiles retention instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, I was wondering about this as well. Some quick research showed me that the monolith builds a project config, which then gets propagate to relay. And that config is coming from getsentry. At this point I feel like pulling on a thread a bit 😅 but I guess we need this, maybe @jjbayer could chime in here as well, before I start adding support for this.

We would need to

  1. [getsentry] Propagate the config based on the plan, looks like this already happens automatically as it iterates over all data categories
  2. [sentry] Extend the RETENTIONS_CONFIG_MAPPING to also include DataCategory.PROFILE_DURATION and DataCategory.PROFILE_DURATION_UI, so those get propagated to relay
  3. [relay] Extend RetentionsConfig and use that config here

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.

That sounds sensible, yes.

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.

The retention is already read from passed in by forward_store via StoreRawProfile::retention, and used in handle_raw_profile.

The fact that profiles don't have a custom retention config is out of scope of this PR IMO.

TL;DR: A hard-coded constant is fine for the fallback, but I would define it in a custom constant and set it to 90 days to match the default event retention.

Comment thread relay-server/src/services/objectstore.rs
Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 907aa48. Configure here.

let trace_attachments = Usecase::new("trace_attachments")
.with_expiration_policy(ExpirationPolicy::TimeToLive(DEFAULT_ATTACHMENT_RETENTION));
let profiles = Usecase::new("profiles_raw")
.with_expiration_policy(ExpirationPolicy::TimeToLive(DEFAULT_ATTACHMENT_RETENTION));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Wrong retention constant used for profile objectstore uploads

Medium Severity

The profiles usecase is initialized with DEFAULT_ATTACHMENT_RETENTION (30 days), which is the retention for event attachments. Profiles typically use event retention (90 days via DEFAULT_EVENT_RETENTION). While each upload call sets a per-request expiration policy via upload_bytes, the Usecase-level default acts as a fallback. If the per-request policy is ever not applied (e.g. retention_days * 24 overflows u16 via checked_mul returning None), stored raw profiles would be garbage-collected after 30 days instead of the correct profile retention, causing silent data loss. This was also flagged by a reviewer in the PR discussion.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 907aa48. Configure here.

Self::Envelope => "envelope",
Self::EventAttachment => "attachment",
Self::TraceAttachment => "attachment_v2",
Self::RawProfile => "profiles_raw",
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.

All the other strings are in the singular.

Suggested change
Self::RawProfile => "profiles_raw",
Self::RawProfile => "profile_raw",

let trace_attachments = Usecase::new("trace_attachments")
.with_expiration_policy(ExpirationPolicy::TimeToLive(DEFAULT_ATTACHMENT_RETENTION));
let profiles = Usecase::new("profiles")
.with_expiration_policy(ExpirationPolicy::TimeToLive(DEFAULT_ATTACHMENT_RETENTION));
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.

That sounds sensible, yes.

Self::Envelope => "envelope",
Self::EventAttachment => "attachment",
Self::TraceAttachment => "attachment_v2",
Self::RawProfile => "profiles_raw",
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.

Other variants use singular:

Suggested change
Self::RawProfile => "profiles_raw",
Self::RawProfile => "raw_profile",

Comment on lines +263 to +265
if let Some(unsent) = s.try_send_to_objectstore(msg) {
s.send_to_store(unsent.map(|profile, _| profile.store_message));
}
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.

We only need this branch if we want to support perfetto profiles in self-hosted, which has no objectstore yet by default. In that case, we also need the kafka consumer to handler the raw_profile_object_store_key correctly.

So the simpler approach would be to never send raw profiles via kafka, and accept that perfetto is not enabled in self-hosted.

raw_profile: chunk.raw_profile,
}));
if chunk.raw_profile.is_some() {
let msg = chunk.map(|chunk, _| {
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.

It should be possible to move the if into the map and then use if let Some(raw_profile) = chunk.raw_profile.

Comment on lines +161 to +164
/// Objectstore key where the raw profile blob is stored.
pub raw_profile_object_store_key: Option<String>,
/// Content type of the raw profile (e.g. Perfetto trace).
pub raw_profile_content_type: Option<ContentType>,
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.

Since a StoreProfileChunk can only have either a byte payload or an objectstore key, I would encode this into the type system as something like

pub payload: StoreProfileChunkPayload

where

enum StoreProfileChunkPayload {
    Bytes(Bytes),
    ObjectstoreKey(String)
}

raw_profile: message.raw_profile.as_ref().map(|r| r.payload.clone()),
raw_profile_content_type: message.raw_profile.map(|r| r.content_type),
raw_profile_object_store_key: message.raw_profile_object_store_key,
raw_profile_content_type: message.raw_profile_content_type,
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.

Why do we need to provide the content type in the kafka message in the first place? Isn't it always ContentType::PerfettoTrace?

/// Content type of the raw profile.
pub content_type: ContentType,
/// The profile chunk message to forward to Kafka after upload.
pub store_message: StoreProfileChunk,
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.

This looks unnecessarily nested. The objectstore service should be able to construct the StoreProfileChunk message from scratch when it sends the data to the store service.

let trace_attachments = Usecase::new("trace_attachments")
.with_expiration_policy(ExpirationPolicy::TimeToLive(DEFAULT_ATTACHMENT_RETENTION));
let profiles = Usecase::new("profiles")
.with_expiration_policy(ExpirationPolicy::TimeToLive(DEFAULT_ATTACHMENT_RETENTION));
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.

The retention is already read from passed in by forward_store via StoreRawProfile::retention, and used in handle_raw_profile.

The fact that profiles don't have a custom retention config is out of scope of this PR IMO.

TL;DR: A hard-coded constant is fine for the fallback, but I would define it in a custom constant and set it to 90 days to match the default event retention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants