Introduce instruction_addr_adjustment Stack Trace Attribute

[Swatinem]

This RFC introduces a new StackTrace Protocol field that controls adjustment of
the instruction_addr for symbolication.

Rendered RFC

[mitsuhiko]

I though the deduplication actually happens on the server side. Either way though it sounds like profiling probably wants to do the heuristic manually. That said I wonder if the new adjustment is a question for the ingestion protocol and symbolicator or if in symbolicator this should be handled separately.

I am wondering if form the perspective of the symbolicator interface it wouldn't make more sense to add a is_first flag into every single frame into the symbolication payload. That way profiling could deduplicate into (is_first, addr) and still use the auto mode.

[Swatinem]

I think it depends on the SDK. It does deduplication in Rust at least:

https://github.com/getsentry/sentry-rust/blob/30b949d0b1e08b2e6cb45a715567fbdf007e7409/sentry-core/src/profiling.rs#L182-L193

IMO, if we decide to make this a per-frame attribute, I would rather just put a adjust_instruction_addr to the frame. The default would depend on leaf-frame and heuristic, and an SDK can chose to set this explicitly to true. Though we would have the same problem as with in_app that we have to default stuff based on the presence of any frame that has this property set?

Ideally the SDK should always send unadjusted (original) instruction_addr, and the server should always adjust those, unless we assume its a leaf frame.

[mitsuhiko]

Just to be clear I would only make it a per-frame attribute on the symbolication side (sentry -> symbolicator).

[Swatinem]

After a discussion with the profiling team, we came to the following conclusion:

• Implement the SDK -> Sentry protocol as proposed in this RFC.
• Implement a per-frame "needs adjustment" boolean flag in the internal Sentry -> Symbolicator protocol.
• For backwards compatibility, the instruction_addr_adjustment: "auto" (or undefined) will not add any per-frame attributes in the Sentry -> Symbolicator call.
• If none of the frames has the per-frame attribute set, symbolicator will default to the existing "auto" behavior. (eg. let use_auto = frames.iter().all(|frame| frame.should_adjust.is_none());)
• The default will be "needs adjustment", so ideally only the first frame in Sentry->Symbolicator stack traces needs to be marked.
• Profiling should mark frames as "does not need adjustment" by marking every top frame in the stacks, eg: stack_traces.iter().flat_map(|frames| frames.first()).for_each(|frame_idx| global_frame_list[frame_idx].needs_adjustment = Some(false))
• Sorry for the Rust-style oneliners :-)

[Swatinem]

Another thing I just noticed: We are only doing the adjustment for absolute addresses, and NOT for relative addressing mode (aka WASM).
I believe that is the correct thing to do, right?

[Swatinem]

@bitsandfoxes we had a look at most of the events you sent us, they look consistent across unity versions on one platform, but different across platforms.

Here is what we found out:

• On Android arm64 (across all editor versions), Unity reports already adjusted addresses (pointing to the call instruction), so no further adjustment on our end needs to happen
  -> SDK should report instruction_addr_adjustment: "none"
• On macOS arm64 and iOS x86_64 (simulator build?), Unity reports return addresses for all the frames, so symbolicator needs to adjust all of them
  -> SDK should report instruction_addr_adjustment: "all"

It would be nice to have one example of a Windows event / binary as well to take a look at the reported address in relation to the disassembly. Then we know which adjustment mode the SDK should report.