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

Rebase of PR #7556 #7803

Merged
merged 3 commits into from
Nov 13, 2024
Merged

Conversation

rustyrussell
Copy link
Contributor

I think this:

Closes: #7575
Closes: #7556 ?

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

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

ACK 73e83c6

@cdecker
Copy link
Member

cdecker commented Nov 12, 2024

This might require an update mocks call, the flake build is failing in run route overlong not having the mocks for tracing related functions.

Copy link
Contributor

@JssDWt JssDWt left a comment

Choose a reason for hiding this comment

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

One nit, that I thought I had fixed in #7556, but apparently not.


/* Annotate the JSON-RPC span whether it succeeded or failed,
* and then emit it. */
trace_span_tag(out, "error", contenttok == NULL ? "true" : "false");
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be the other way around

Suggested change
trace_span_tag(out, "error", contenttok == NULL ? "true" : "false");
trace_span_tag(out, "error", contenttok == NULL ? "false" : "true");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's two negatives. I will change it to contenttok ? "true" : "false"

cdecker and others added 2 commits November 13, 2024 14:52
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.
Changelog-Added: Plugins: `pay` now has tracing support for various payment steps.
Copy link
Contributor

@JssDWt JssDWt left a comment

Choose a reason for hiding this comment

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

ACK b65fcbe

@vincenzopalazzo
Copy link
Collaborator

Unrelated faulure inside the CI

lightningd-1 2024-11-13T07:15:15.499Z DEBUG   gossipd: Shutting down
lightningd-1 2024-11-13T07:15:15.532Z DEBUG   hsmd: Shutting down
{'github_repository': 'ElementsProject/lightning', 'github_sha': '254d079e69501119aae75f7ea200d5bc43c7c6da', 'github_ref': 'refs/pull/7803/merge', 'github_ref_name': 'HEAD', 'github_run_id': 11811119714, 'github_head_ref': 'guilt/pr-7556', 'github_run_number': 11470, 'github_base_ref': 'master', 'github_run_attempt': '2', 'testname': 'test_important_plugin', 'start_time': 1731482015, 'end_time': 1731482115, 'outcome': 'success'}
----------------------------- Captured stderr call -----------------------------
Lost connection to the RPC socket.Reading JSON input: Connection reset by peerReading JSON input: Connection reset by peerReading JSON input: Connection reset by peer
--------------------------- Captured stdout teardown ---------------------------
['lightningd-1 2024-11-13T07:14:12.293Z **BROKEN** plugin-spenderp: Lost connection to the RPC socket.']
===Flaky Test Report===

merging this, and opening an issue for the CI faulure

@vincenzopalazzo vincenzopalazzo merged commit 087a29b into ElementsProject:master Nov 13, 2024
30 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants