rpc,internal/telemetry: fix deferred spanEnd to capture errors via pointer

[jrhea]

Summary

The endSpan closure accepted error by value, meaning deferred calls like defer spanEnd(err) captured the error at defer-time (always nil), not at function-return time. This meant errors were never recorded on spans.

Changes

• Changed endSpan to accept *error
• Updated all call sites in rpc/handler.go to pass error pointers, and adjusted handleCall to avoid propagating child-span errors to the parent
• Added TestTracingHTTPErrorRecording to verify that errors from RPC methods are properly recorded on the rpc.runMethod span

[jrhea]

cc @lightclient

[MariusVanDerWijden]

LGTM, but it kinda feels like we are fixing the API because people are using it incorrectly, but I guess its okay

[jrhea]

LGTM, but it kinda feels like we are fixing the API because people are using it incorrectly, but I guess its okay

if you ask me, I should have done it this way from the beginning. the options were to either make spanEnd() accept a pointer to an error or we need to do something ugly like defer func() { spanEnd(err) }().

I made this example in go playground to demonstrate:

https://go.dev/play/p/GYTfThDS-Y2

[lightclient]

LGTM.

Sorry this API with the pointer error really threw me when I was reviewing originally, so I pushed Jonny to remove. But in retrospect now, I see that you definitely want to be able to defer the spanEnd calls, so this is needed.

[lightclient]

server span doesn't propagate error anymore, eh?

[jrhea]

good catch. i think that part of the comment needs to be removed.