feat(profiles): Add support for continuous profiling#3270
Conversation
jjbayer
left a comment
There was a problem hiding this comment.
Looks good overall! Could you please add an integration test to test_store.py to make sure the entire pipeline works as expected?
| item.set_payload(ContentType::Json, payload); | ||
| ItemAction::Keep | ||
| } else { | ||
| ItemAction::Drop(Outcome::Invalid(DiscardReason::Profiling( |
There was a problem hiding this comment.
We should update Item::quantity() to avoid creating outcomes that default to quantity=1:
relay/relay-server/src/envelope.rs
Lines 646 to 651 in b6211aa
There was a problem hiding this comment.
I don't know if we have a good quantity to set here. Since quantity is now the duration of a profile, it means we need to first parse and process the profile properly, and since Relay won't emit Accepted outcomes for profile chunks and only invalid ones, how useful it is to try to process the profile to determine the appropriate duration we failed to ingest?
It will make some charts look a bit weird since it won't be the same unit for valid and invalid profile chunks. I could still try to get a duration but we would still have cases where the profile chunk is malformed or sending bogus data and we can't determine the duration of the profile chunk.
There was a problem hiding this comment.
After a short discussion with @Dav1dde, I believe the only correct solution is having two different outcomes: one for the number of profile chunks and one for the duration. Not necessarily a blocker for this PR, but I think it's a change we should make.
Dav1dde
left a comment
There was a problem hiding this comment.
Only a quick check without checking logic.
aa31f30 to
e084069
Compare
jjbayer
left a comment
There was a problem hiding this comment.
I would still add an integration test before we merge this, apart from that this looks good to me.
| item.set_payload(ContentType::Json, payload); | ||
| ItemAction::Keep | ||
| } else { | ||
| ItemAction::Drop(Outcome::Invalid(DiscardReason::Profiling( |
There was a problem hiding this comment.
After a short discussion with @Dav1dde, I believe the only correct solution is having two different outcomes: one for the number of profile chunks and one for the duration. Not necessarily a blocker for this PR, but I think it's a change we should make.
Co-authored-by: Joris Bayer <joris.bayer@sentry.io>
This aims to add support for continuous profiling.
If the continuous profiling option is enabled, the SDK will start a profiler at the same time it starts. The profiler will generate a random
profiler_idand start profiling in chunks of a fixed duration (likely 10s) and send each chunk to Relay.The chunks will be handled as a separate
ItemTypeand have a separateDataCategory. The schema of the payload is our Sample format V2, resembling V1 but incompatible with it. Changes are:f64instead of aStringand renamedtimestamp_msprofiler_idfield representing a profiling sessionEach chunk will then be sent through the same topic to our consumers for further processing and storage.
Acceptedoutcomes based on the duration of each chunk in milliseconds will be emitted there.