Preliminary work to integrate Performance Monitoring

[aramperes]

Hello! I'm the lead developer at Wavy Labs Inc. We're integrating Rust more and more into our stack and we've been using Sentry for over a year. We'd love to start integrating Performance Monitoring, but it seems this SDK doesn't support it yet.

I'd like to kickstart the discussion on this and contribute some developer time for this. I've been reading the Guidelines for Tracing Support and peeking into the Python SDK for reference. I've started some preliminary work on adding the new types (Span and Transaction), but I'll need some guidance on integrating with Hub. Feel free to start reviewing at any time!

[Swatinem]

I really appreciate you taking first steps here! Tracing is on our radar, we just wanted to gather more experience with our other SDKs before integrating it into Rust together will all the learnings from the other SDKs. That being said, I haven’t really looked at the implementation from other SDKs, or how it would best fit into Rust.

Nevertheless, looking at your draft and the Guidelines, I think that the Transaction type should rather live in sentry-core, as it should be able to "submit itself" to the client/transport. So it needs to have at least an Arc<Client> on itself.

Instead of "extending" the existing Event type, it should rather use composition, as in:

struct Transaction {
event: Event,
span: Span,
client: Arc<Client>,
}

It can then flatten all the event/span properties when serializing.
It can also have a DerefMut impl for both Event and Span.

I would also give it its own EnvelopeItem type.

Roping in @HazAT and @rhcarvalho as they should have more experience with the tracing API.

[aramperes]

Glad to help! I understand that this may not have been on your team's roadmap in the immediate future. Nevertheless we'll try to contribute as much as possible.

Since EnvelopeItem lives in sentry-types, and Client lives in sentry-core, I don't think it's possible to include a client reference in Transaction.

This seems more like the sort of thing that could use a trait and Hub::current, e.g.

// In `sentry-types`

#[derive(Clone, Debug, Serialize)]
pub struct Transaction<'a> {
    event: Event<'a>,
    span: Span,
}

// In `sentry-core`

// Trait in case other types of envelopes can 'submit themselves'
pub trait Sendable { // [or another name]
	pub fn send();
}

impl<'a> Sendable for Transaction<'a> {
    fn send(&self) {
        Hub::current().send_transaction(&self)
    }
}

Evidently, apps would have to import the Sendable trait to be able to dispatch transactions like this.

I'm a bit flakey on the terminology (finishing, dispatching, sending, etc.) so feel free to correct me.

[Swatinem]

This looks much better now.

Ah yes, I moved all the Envelope-related things into sentry-types. We had a similar case with Session, and decided to split the POD-bits into the types crate, and a small wrapper into core. See #251
I think something similar can be done here as well.

This PR on its own looks very promising already, and I would be happy to land this incrementally, meaning landing the bits from types even if they are not yet fully usable.

[aramperes]

Sounds good!

How about I finish this PR by:

• implementing TraceContext
• documenting the new structs and fields
• adding unit tests for serializing Transaction and Span to make sure everything is proper
• reverting the changes in Scope to be done in a later PR

[Swatinem]

Yes that sounds very good

[rhcarvalho]

FWIW we've updated the developer docs to include the tracesSampler option that gives more control over the sampling decision. Not a must have for a first pass, but something to keep in mind.

https://develop.sentry.dev/sdk/unified-api/tracing#sdk-configuration

(might be easier to read the diff -- getsentry/develop#165)

[aramperes]

@rhcarvalho Thanks for the update! I went through it and it's much clearer than before.

Just to double-check that this is essentially what the JSON structure of the envelope looks like for a Transcation event:

// Envelope Headers
{"event_id": "...", "sent_at": "..."}

// Item Headers
{"type": "transaction", ...}

// Item payload 
{
    // Transaction struct
    "type": "transaction",
    "event_id": "...",
    "sdk": "...",
    "platform": "...",
    "transaction": "...",
    "tags": { ... },
    "timestamp": "...",
    "start_timestamp": "...",
    "spans": [
        { // Span struct
            "trace_id": "...",
            "span_id": "...",
            "parent_span_id": "...",
            "op": "...",
            "description": "...",
            "timestamp": "...",
            "start_timestamp": "...",
            "same_process_as_parent": false,
            "status": "...", // Optional
            "tags": { ... }, // Optional
            "data": { ... }, // Optional
        },
        // ...
    ],
    "contexts": {
        "trace": { // TraceContext struct
            "trace_id": "...",
            "span_id": "...",
            "parent_span_id": "...",
            "op": "...",
            "description": "...",
            "status": "..." // Optional
        }
    }
}

This makes the Transaction struct a bit more complex than inheriting Span + Event. It's essentially a Span, but some fields are at the top level, and others are in TraceContext. Others are omitted (data, same_process_as_parent). It also has a type field always set to "transaction", plus some Event fields (event_id, sdk, platform, transaction). The meaning of timestamp is also swapped from Event: it becomes the end timestamp, and start_timestamp replaces the purpose of the timestamp field.

Makes me think Transaction, Event, Span, and TraceContext should just be their own struct instead of merging types together.

Another approach, with inheriting Event (although initialising this type could be a bit awkward):

pub struct Transaction<'a> {
   #[serde(flatten)]
   event: Event<'a>,

   type: Option<String>, // Always `"transaction"` -- possibly an enum and move into `Event`
   spans: Vec<Span>,
   start_timestamp: DateTime<Utc>,
}

In that case, there's a complication because Event#timestamp is set when the event is created, whereas it should be undefined until the Transaction is completed. This would make timestamp an Option property. Could also have a field end_timestamp instead of start_timestamp, and use timestamp as start_timestamp, but serializing would have to handle this case.

[Swatinem]

It also has a type field always set to "transaction"

I’m actually not sure if that is supposed to live on the transaction type itself, or if its meant to be just the envelope header.

[aramperes]

I'll exclude it, but it does appear to be sent in other SDKs. It is used to identity the type of event in SDKs that don't have strict typing, e.g. in Python 'Event' is just a Dict, so this field is used to send the envelope differently. It wouldn't be necessary in Rust since Transaction has a different struct.

https://github.com/getsentry/sentry-python/blob/91c7a8fcb8e94b37e7dba74e66f7d0992f3cf145/sentry_sdk/client.py#L337-L352

It's possible that it gets ignored on the server-side however.

[Swatinem]

after talking to @rhcarvalho, it looks like we need a type on the transaction object after all (at least when its serialized to json) I will research this myself to know how this should look like

[Swatinem]

Sorry for the delay here. Please rebase this on master again, the clippy error should be gone then.

This looks good from my perspective, I will ask @rhcarvalho for another review however, as he should know all the data structures related to tracing a bit better than me.

[Swatinem]

Thanks a lot for doing this!

I’m wondering if we should maybe gate all of this behind an unstable feature flag, to indicate that we would want to iterate on these types a bit more before we consider them to be "stable" before we put them in a release.