Update the Transaction struct to match the transaction payload spec

[leops]

There are two main changes here to fix errors displayed on the Sentry dashboard when submitting envelopes using the current Transaction struct:

• First the name field is renamed to transaction, this is technically a semver breaking change although given there is not high-level API to work with transactions yet in the SDK this struct probably doesn't have too many consumers yet. If that proves to be a problem it's always possible to just rename the field at the serde level
• Second, the span_id fields are now using a custom serializer to truncate the Uuid string at 16 bytes. I went for the simplest change here again, although it means using the Uuid type for fields that are not in fact holding a full UUID in the wire format. Maybe the span IDs should be either a String (like in parent_span_id) or a specialized SpanId struct of only 16 bytes

[Swatinem]

lgtm, I will check back with the performance team for these changes.

[rhcarvalho]

Without diving into the code base, it seems from these changes that there are some gaps in the current implementation.

The change in length of span_id is correct, but span_id (nor trace_id) should NOT be UUIDs.

Renaming Transaction.name -> Transaction.transaction is incorrect.

[rhcarvalho]

Hmm span_id is not supposed to be an UUID, and specially not a truncated UUID, essentially throwing away the random bits.

Unfortunately a documentation rendering bug is causing the spec for span_id not to show in https://develop.sentry.dev/sdk/event-payloads/span/

The source is https://github.com/getsentry/develop/blob/master/src/docs/sdk/event-payloads/span.mdx and https://github.com/getsentry/develop/blob/master/src/docs/sdk/event-payloads/properties/span_id.mdx

The spec for span_id and trace_id match those from OpenTelemetry and are, respectively, 64 and 128 random bits encoded as 16 and 32 hex chars.

[leops]

Alright I'll introduce new SpanId and TraceId types defined as fully random [u8; 16] and [u8; 32] respectively for these fields (and parent_span_id as well I guess ?)

[rhcarvalho]

If we talk bytes, u8, SpanID (and ParentSpanID) is 8 bytes and TraceID is 16 bytes, so we might use smaller arrays and hex-encode them when serializing.

[rhcarvalho]

The transaction itself has a name, not to be confused with how the transaction gets transformed into an event before sending to Sentry. Transaction.name is mapped into Event.transaction.

[leops]

I might have been confused here since the module is called protocol, but these definitions don't actually map one-to-one to the payloads that are sent to the store endpoint, is that correct ? If I understand correctly there should be a piece of code (currently unimplemented) that turns a Transaction struct into an Event instead of it being serialized and submitted directly ?

[rhcarvalho]

Correct, the in-memory representation is different than the over-the-wire representation.

[leops]

I pushed a proper fix for the span_id and trace_id fields to use new SpanId and TraceId types defined respectively as fully random [u8; 8] and [u8; 16] that serialize to hex strings. This required introducing 2 new dependencies, one on the getrandom library (this one doesn't actually change anything since it's the same dependency used to generate v4 UUID anyway) and one on the hex library as I preferred not to get into the business of implementing hex string parsing.

For the name field the story is a little more complicated since as I understand it using the high-level startTransaction API calling finish on a Transaction would turn it into an Event and send it through capture_event on the current Hub. However this part of the API is currently unimplemented (I guess due to the difficulty of conciliating the Span tree builder pattern with the rigid ownership rules in Rust ?), and what the SDK currently has instead is a direct translation of the Transaction into an Envelope object, without going through the intermediate Event. Down the line it means the Transaction struct is directly serialized into a transaction envelope item, and more or less has to masquerade as an Event object at least as far as the serialization code is concerned. For now I simply restored the name field but renamed it to transaction on serialization, but according to the SDK development documentation it might be better to remove the implementation of the From<Transaction> for Envelope translation and implement a From<Transaction> for Event transform instead ?

[Swatinem]

it might be better to remove the implementation of the From<Transaction> for Envelope translation and implement a From<Transaction> for Event transform instead ?

That would be a breaking change either way. I think we can go ahead with this as-is, and gather some feedback on how it feels.

[codecov-commenter]

Codecov Report

Merging #349 (37fceed) into master (81e133f) will increase coverage by 0.04%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
+ Coverage   86.58%   86.62%   +0.04%     
==========================================
  Files          66       66              
  Lines        6848     6892      +44     
==========================================
+ Hits         5929     5970      +41     
- Misses        919      922       +3