feat(profiling): Add a Stacktrace ItemType to represent the stacktraces sent from Sentry SDKs #1179
Conversation
relay-server/src/envelope.rs
Outdated
| @@ -106,6 +106,8 @@ pub enum ItemType { | |||
| ProfilingSession, | |||
| /// Profiling data | |||
| ProfilingTrace, | |||
| /// Stacktrace event payload encoded in JSON | |||
| Stacktrace, | |||
There was a problem hiding this comment.
A general question on the naming of this: Does this still contain profiling/specto-specific payloads? If so, it would be great if we could reflect this in the name of this item type somehow.
In parts, my thinking here is that we also have a stack trace schema in Sentry's error events. It would be good if we either separate the two clearly, including by using different terminology, or alternatively use the same schema. Choosing the second option is clearly a larger discussion.
Same applies to all other references in this PR, such as the Kafka topic assignment.
There was a problem hiding this comment.
It does not contain anything Specto specific, it's only profiling related now. You're not the only one mentioning this so I decided to change the name to "profile". I think it's a better name overall since it's easy to think "profiling team manages profiles" and it doesn't collide with other names in other repos (except maybe user profiles but it's so unrelated, I think it's OK).
There was a problem hiding this comment.
That said, it probably still makes sense somewhat aligning the fields so SDKs and backend code does not have to deal with two stacktrace formats. Particularly if you plan on forwarding this to symbolicator.
There was a problem hiding this comment.
I guess we do have the timing information for each address, so we can't 100% match that format. We'll use a superset of that format, I'll need to transform what I get anyway to send the addresses to symbolicator but keep the original format.
It's going to look like that:
{
"samples": [
{
"timestamp": <some duration from start of transaction>,
"thread_id": "<some uint64 thread ID>",
"frames": [
{ "instruction_addr": "0x7fff5bf3456c" },
{ "instruction_addr": "0x7fff5bf346c0" }
...
],
},
...
]
}
phacops
force-pushed
the
pierre/profiling-stacktrace-item-type
branch
from
806853d
to
e9bac93
Compare
relay-server/src/envelope.rs
Outdated
| @@ -106,6 +106,8 @@ pub enum ItemType { | |||
| ProfilingSession, | |||
| /// Profiling data | |||
| ProfilingTrace, | |||
| /// Stacktrace event payload encoded in JSON | |||
| Stacktrace, | |||
There was a problem hiding this comment.
It does not contain anything Specto specific, it's only profiling related now. You're not the only one mentioning this so I decided to change the name to "profile". I think it's a better name overall since it's easy to think "profiling team manages profiles" and it doesn't collide with other names in other repos (except maybe user profiles but it's so unrelated, I think it's OK).
phacops
force-pushed
the
pierre/profiling-stacktrace-item-type
branch
from
e9bac93
to
4f41d69
Compare
|
As per @phacops, this PR should be ready. I'll give it a read today 👍 |
phacops
force-pushed
the
pierre/profiling-stacktrace-item-type
branch
from
451fbb0
to
9b92ae0
Compare
…iling Kafka message
| @@ -821,6 +826,7 @@ impl Default for TopicAssignments { | |||
| metrics: "ingest-metrics".to_owned().into(), | |||
| profiling_sessions: "profiling-sessions".to_owned().into(), | |||
| profiling_traces: "profiling-traces".to_owned().into(), | |||
| profiles: "profiles".to_owned().into(), | |||
There was a problem hiding this comment.
Please remember to deploy the prod config in getsentry/ops before deploying this PR.
For phase 1, I added 2
ItemTypein a previous PR to allow ingesting Specto payloads. Those payloads were a session containing information about the device and a trace containing metrics about cpu, memory, network usage and a stack trace. This was ingested via a Specto service into Spanner.For phase 2, we will implement our profiling code into Sentry SDKs and we now only have a stack trace to ingest via a Snuba worker. This is why we need to implement a new
ItemTypesince it's a very different type of data (JSON payload instead of protobuf and containing only the stack trace and no other metric).After our work for phase 2 on the SDKs is released, we'll be able to drop whatever we implemented for phase 1.