Skip to content

xpay: add traces for payments - #9137

Open
Lagrang3 wants to merge 3 commits into
ElementsProject:masterfrom
Lagrang3:xpay-traces
Open

xpay: add traces for payments#9137
Lagrang3 wants to merge 3 commits into
ElementsProject:masterfrom
Lagrang3:xpay-traces

Conversation

@Lagrang3

@Lagrang3 Lagrang3 commented May 14, 2026

Copy link
Copy Markdown
Collaborator

Produce traces (see common/trace.h) for every payment in order to gather statistics about the duration of a full payment execution.

xpay doesn't spend much time on computation itself. The runtime bottlenecks are always rpc calls to either askrene or lightningd's pay primitives.

The trace hierarchy here takes the following form:

  • xpay/fetchinvoice: stand alone trace for the operation of fetching the invoice if xpay receives a BIP353 address or an offer.
  • xpay/payment: a whole payment
    -> xpay/getroutes: waiting for askrene-getroutes,
    -> xpay/injectpaymentonion: waiting for the payment onion to return.

@Lagrang3
Lagrang3 force-pushed the xpay-traces branch 3 times, most recently from 167792d to 2d28e46 Compare May 21, 2026 06:42
@Lagrang3
Lagrang3 requested a review from cdecker May 21, 2026 07:27
@Lagrang3

Lagrang3 commented May 21, 2026

Copy link
Copy Markdown
Collaborator Author

@cdecker, are these trace annotations useful? Do you think I should add more tags?

@Lagrang3
Lagrang3 marked this pull request as ready for review May 21, 2026 07:32
cdecker
cdecker previously approved these changes May 23, 2026

@cdecker cdecker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice change. I'm not sure the back-to-back suspends in line 1485 is correct, but it doesn't matter as all it does is to unset the thread's context.

@Lagrang3

Copy link
Copy Markdown
Collaborator Author

Very nice change. I'm not sure the back-to-back suspends in line 1485 is correct, but it doesn't matter as all it does is to unset the thread's context.

Yes, please check.
At line 1882: resume payment, brings it into current trace
next: create new trace getroutes with payment as parent, now getroutes is current trace
next: suspend getroutes (we must before rendering execution to the io loop), now payment (the parent) is current,
next: suspend payment

@Lagrang3

Copy link
Copy Markdown
Collaborator Author

rebased

@Lagrang3
Lagrang3 force-pushed the xpay-traces branch 3 times, most recently from bee28e3 to 44066a2 Compare July 23, 2026 08:44
@Lagrang3

Copy link
Copy Markdown
Collaborator Author

This test: tests/test_askrene.py:test_reservations_leak_under_load was hitting an access after free of a trace's parent.
I couldn't find the bug but refactored the traces. I think the new version is cleaner.
We keep a root trace for each payment instance, then child traces are added for each call to getroutes and injectpaymentonion. I did not add new code to start and end the traces because libplugin already does that for every jsonrpc
call. I needed only to attach the jsonrpc call trace to parent payment.

@Lagrang3
Lagrang3 requested a review from cdecker July 23, 2026 12:38
Produce traces (see common/trace.h) for every payment in order to gather
statistics about the duration of a full payment execution.

Changelog-None

Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Lagrang3 added 2 commits July 27, 2026 09:22
Changelog-None

Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
Changelog-None

Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
@Lagrang3

Copy link
Copy Markdown
Collaborator Author

I think I fixed this finally.
trace has a simple interface, it also fits perfectly when your application is single threaded.
Here to correctly trace concurrent payment executions you would need to do awkward things.

For example I wanted to measure the time a payment spends waiting for injectpaymentonion.
Before sending the RPC request out we do the following:

// global current trace is assumed to be NULL
// we get control back from the io loop, we resume the current payment
+       trace_span_resume(attempt->payment); // payment is the parent span
// start a new sub-trace for the rpc
+       trace_span_start("xpay/injectpaymentonion", attempt);
+       trace_span_tag(attempt, "partid",
+                      tal_fmt(attempt, "%d", (int)(attempt->partid)));
// before we hand control back to the io loop, we unwind the trace stack
+       trace_span_suspend(attempt);
+       trace_span_suspend(attempt->payment);
// global current trace is back to NULL

in the RPC callback, both error and happy path we do:

// global current trace is assumed to be NULL
// we get control back from the io loop, we resume the current payment
+       trace_span_resume(attempt->payment);
// also resume the trace of the RPC call
+       trace_span_resume(attempt);
// end the RPC trace
+       trace_span_end(attempt);
// again unwind the rest of the trace stack with only one element
+       trace_span_suspend(attempt->payment);
// global current trace is back to NULL

@madelinevibes madelinevibes added Status::Ready for Review The work has been completed and is now awaiting evaluation or approval. and removed PLEASE clear CI 🫠 labels Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status::Ready for Review The work has been completed and is now awaiting evaluation or approval.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants