Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trace payment_continue and payment mods #7556

Closed

Conversation

JssDWt
Copy link
Contributor

@JssDWt JssDWt commented Aug 12, 2024

No description provided.

@cdecker
Copy link
Member

cdecker commented Aug 15, 2024

Thanks @JssDWt, I've been wanting to instrument this part of the code for quite a while, and never had the muse to get started. I added #7575 on top of your change to instrument all JSON-RPC calls that plugins start, and we can likely also instrument all entrypoints to the plugin as well (rpcmethod, hook and notifications).

@JssDWt JssDWt force-pushed the jssdwt-payment-trace-attempt-2 branch from dd941ac to c25f23e Compare August 15, 2024 16:43
plugins/libplugin-pay.c Outdated Show resolved Hide resolved
cdecker and others added 3 commits August 16, 2024 13:07
The `send_outreq` function is a good place to suspend and resume
traces, since these are usually the places where we hand off control
back to the `io_loop`. This assumes that we do not continue doing
heavy liftin after we have queued an `outreq` call, but that is most
likely the case anyway. This frees us from having to track suspensions
whenever we call the RPC from a plugin.
@JssDWt JssDWt force-pushed the jssdwt-payment-trace-attempt-2 branch from c25f23e to e0ce284 Compare August 16, 2024 11:07
@JssDWt
Copy link
Contributor Author

JssDWt commented Aug 16, 2024

@cdecker I moved the commits around a little bit. Only tracing payment_continue now, because under some circumstances it seemed that the traces got intertwined. Also incorporated your suggestion here

It now builds on the (modified) commit from #7575

@JssDWt JssDWt marked this pull request as ready for review August 16, 2024 11:15
@JssDWt JssDWt requested a review from cdecker August 16, 2024 11:15
@JssDWt
Copy link
Contributor Author

JssDWt commented Oct 31, 2024

Rebased. And also fixed a bug where the error field would be false when there was an error and vice versa.

@rustyrussell rustyrussell added this to the v24.11 milestone Nov 12, 2024
@rustyrussell
Copy link
Contributor

Pretty! I missed this on the milestone, fixed now.

Rebasing now.

@rustyrussell
Copy link
Contributor

This is confusing. #7575 says it builds on this, this says it's now based on that. And I can't force push to this branch to rebase.

So I'm going to add to the confusion by opening another PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants