tracing: first tracepoints and documentation on User-Space, Statically Defined Tracing (USDT)

0xB10C commented at 5:16 pm on May 20, 2021: contributor

This PR adds documentation for User-Space, Statically Defined Tracing (USDT) as well as three tracepoints (including documentation and usage examples).

Context

The TRACEx macros for tracepoints and build system changes for USDT were merged in #19866 earlier this year. Issue #20981 contains discussion about potential tracepoints and guidelines for adding them (also documented with this PR). USDT was a topic in a core-dev-meeting discussion on 21st Jan, 2021.

USDT? Stablecoin?

User-space, Statically Defined Tracing (USDT) allows for more observability during development, debugging, code review, and production usage. The tracepoints make it possible to keep track of custom statistics and enable detailed monitoring of otherwise hidden internals and have little to no performance impact when unused. Linux kernels (4.x or newer) can hook into the tracepoints and execute eBPF programs in a kernel VM once the tracepoint is called.

This PR includes, for example, tracepoints for in- and outbound P2P messages.

 0USDT and eBPF Overview
 1======================
 2
 3                ┌──────────────────┐            ┌──────────────┐
 4                │ tracing script   │            │ bitcoind     │
 5                │==================│      2.    │==============│
 6                │  eBPF  │ tracing │      hooks │              │
 7                │  code  │ logic   │      into┌─┤►tracepoint 1─┼───┐ 3.
 8                └────┬───┴──▲──────┘          ├─┤►tracepoint 2 │   │ pass args
 9            1.       │      │ 4.              │ │ ...          │   │ to eBPF
10    User    compiles │      │ pass data to    │ └──────────────┘   │ program
11    Space    & loads │      │ tracing script  │                    │
12    ─────────────────┼──────┼─────────────────┼────────────────────┼───
13    Kernel           │      │                 │                    │
14    Space       ┌──┬─▼──────┴─────────────────┴────────────┐       │
15                │  │  eBPF program                         │◄──────┘
16                │  └───────────────────────────────────────┤
17                │ eBPF kernel Virtual Machine (sandboxed)  │
18                └──────────────────────────────────────────┘
19
201. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM
212. The eBPF program hooks into one or more tracepoints
223. When the tracepoint is called, the arguments are passed to the eBPF program
234. The eBPF program processes the arguments and returns data to the tracing script

The two main eBPF front-ends with support for USDT are bpftrace an BPF Compiler Collection (BCC). BCC is used for complex tools and daemons and bpftrace is preferred for one-liners and shorter scripts. Example tracing scripts for both are provided with this PR.

This PR adds three tracepoints:

net:inbound_message
net:outbound_message
valildation:block_connected

See doc/tracing.md and contrib/tracing/ for documentation and example tracing scripts.

Open Questions (Not in scope for this PR)

How to use these tracepoints under macOS?
Release builds with USDT support?
Should and can the tracepoints be automatically tested?

Todo (before undraft)

bcc example showing how to read raw P2P messages up to 32kb
document that you need sys/sdt.h from systemtap for USDT support in Bitcoin Core (apt install systemtap-sdt-dev on debian-like). See https://github.com/bitcoin/bitcoin/pull/19866/commits/933ab8a720cb9b3341adec4109cffb6dc5b322a5
release notes?

laanwj added the label Utils/log/libs on May 20, 2021

0xB10C force-pushed on May 20, 2021

in contrib/tracing/README.md:108 in 92dc3a6fcd outdated

103+   23  11         1248 byte  12         1255 byte  block-relay-only       XX.XXX.XXX.220:8333
104+   24  3           103 byte  1           102 byte  feeler                 XXX.XXX.XXX.64:8333
105+…
106+```
107+
108+Showing recent P2P messages between our onde and their node for a selected peer.

adamjonas commented at 5:34 pm on May 20, 2021:

0Showing recent P2P messages between our node and their node for a selected peer.

jonatack commented at 9:26 am on May 21, 2021:

perhaps omit “their node for "

0xB10C commented at 10:32 am on May 21, 2021:

done both, thanks!

practicalswift commented at 7:36 pm on May 20, 2021: contributor

Concept ACK

Excellent work! :)

adamjonas commented at 9:03 pm on May 20, 2021: member

Concept ACK. Nice work!

jonatack commented at 9:21 am on May 21, 2021: contributor

Concept ACK. Saw a live demo of this and it seems to be very cool.

in contrib/tracing/README.md:113 in 3ca502d16c outdated

108+Showing recent P2P messages between our onde and their node for a selected peer.
109+
110+```
111+    ----------------------------------------------------------------------
112+    |                PEER 16 (4faketorv2pbfu7x.onion:8333)               |
113+    | OUR NODE              outbound-full-relay               THEIR NODE |

jonatack commented at 9:27 am on May 21, 2021:

s/THEIR NODE/PEER/

0xB10C commented at 10:32 am on May 21, 2021:

done, thanks!

in contrib/tracing/README.md:50 in 3ca502d16c outdated

42@@ -43,3 +43,88 @@ example, to point to release builds if needed. See the
43 kernel VM. This means the bpftrace and BCC examples must be executed with root
44 privileges. Make sure to carefully review any scripts that you run with root
45 privileges first!**
46+
47+### log_p2p_traffic.bt
48+
49+A bpftrace script logging information about inbound and outbound P2P network
50+messages. Based on the `net:inbound_message` and `net:outbound_message`

jonatack commented at 9:29 am on May 21, 2021:

Wishlist: this for disconnection and eviction events, with the message, connection direction and type, peer id and address, network the peer connected through.

0xB10C commented at 9:54 am on May 21, 2021:

Agree! We discussed something similar in #20981 (comment) too. Thanks for the input on the arguments! I’ll resolve this in favor of #20981 (comment) :)

0xB10C commented at 5:19 pm on August 12, 2022:

@jonatack there’s #25832 now!

in src/net.cpp:2963 in 92dc3a6fcd outdated

2959@@ -2959,11 +2960,21 @@ bool CConnman::NodeFullyConnected(const CNode* pnode)
2960 void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
2961 {
2962     size_t nMessageSize = msg.data.size();
2963-    LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n",  SanitizeString(msg.m_type), nMessageSize, pnode->GetId());
2964+    auto sanitizedType = SanitizeString(msg.m_type);

jnewbery commented at 10:16 am on May 21, 2021:

Do we need to sanitize this? the message types for all outgoing messages are hardcoded in our binary so should all be safe.

0xB10C commented at 9:37 am on May 24, 2021:

Agree, will remove it from the tracepoint and logging.

jnewbery commented at 10:17 am on May 21, 2021: member

Concept ACK. Excellent work, @0xB10C!

0xB10C force-pushed on May 21, 2021

in doc/tracing.md:30 in cdb4005640 outdated

25+                │  │  eBPF program                         │◄──────┘
26+                │  └───────────────────────────────────────┤
27+                │ eBPF kernel Virtual Machine (sandboxed)  │
28+                └──────────────────────────────────────────┘
29+
30+1. The tracing script complies the eBPF code and loads the eBFP program into a kernel VM

0xB10C commented at 9:06 am on May 22, 2021:

01. The tracing script compiles the eBPF code and loads the eBFP program into a kernel VM

0xB10C commented at 7:13 am on May 24, 2021: contributor

We’ll cover this PR in the PR Review Club this Wednesday (May, 26th).

https://bitcoincore.reviews/22006

0xB10C force-pushed on May 24, 2021

0xB10C commented at 2:28 pm on May 24, 2021: contributor

I added an example (log_raw_p2p_msgs.py) showcasing and documenting some USDT and eBPF limitations. Tracing P2P messages was one of the motivations for USDT in #19866 as an potential alternative to #19509. Some P2P messages like tx and block can be larger than the maximum allocation size of 32kb in eBPF. This means, we can’t submit the complete P2P message to tracing scripts from the eBPF VM. Smaller messages aren’t a problem. Larger messages are cut off after about 32kb in the log_raw_p2p_msgs.py example and a warning is printed.

Something to be aware of when adding tracepoints and developing tracing scripts.

0xB10C force-pushed on May 24, 2021

laanwj commented at 8:26 am on May 25, 2021: member

Impressive work! Thanks for getting back to this.

How to use these tracepoints under macOS?

FWIW I think it’s ok to keep this out of the scope of this PR. Can extend this later. Let’s focus on Linux first.

Release builds with USDT support?

Same for this. I think it’s a no-brainier to have these probes in the release builds eventually, but no need to do so here. Mind that currently we have a duplicate build system (guix and gitian) and the focus for 0.22 (which is not that far away!) is ironing out issues for the guix build, as well as trying to do a guix build for the release. This is a temporary situation that complicates adding anything.

Should and can the tracepoints be automatically tested?

I’d say “yes”. These are meant to be a documented and semi-stable APIs, and APIs not covered by tests tend to code rot over time.

in src/validation.cpp:2007 in a076eb6dc2 outdated

1980+        block.GetHash().ToString().c_str(),
1981+        pindex->nHeight,
1982+        block.vtx.size(),
1983+        nInputs,
1984+        nSigOpsCost,
1985+        GetTimeMicros() - nTimeStart, // in microseconds (µs)

jb55 commented at 12:57 pm on May 25, 2021:

although this is convenient, it does use up a valuable argument slot. we could achieve the same thing with a uprobe:

0uprobe:bitcoind:"CChainState::ConnectBlock(CBlock const&, BlockValidationState&, CBlockIndex*, CCoinsViewCache&, CChainParams const&, bool)" 
1{ [@connect](/bitcoin-bitcoin/contributor/connect/)_start = nsecs;
2}
3
4usdt:bitcoind:validation:block_connected
5{ [@t](/bitcoin-bitcoin/contributor/t/) = nsecs - [@connect](/bitcoin-bitcoin/contributor/connect/)_start;
6}

(untested)

alternatively if we want a more stable interface we could just stick a usdt at the start as well.

0xB10C commented at 11:14 am on May 26, 2021:

I’m not sure if we want to add two tracepoints (one fn start, one fn end) for measuring function timing. This would also require us to pass and handle some kind of identifier for functions running in parallel (not the case for ConnectBlock).

jb55 commented at 12:59 pm on May 25, 2021: contributor

Concept ACK!

in src/validation.cpp:2002 in a076eb6dc2 outdated

1975@@ -1975,6 +1976,16 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
1976     int64_t nTime6 = GetTimeMicros(); nTimeCallbacks += nTime6 - nTime5;
1977     LogPrint(BCLog::BENCH, "    - Callbacks: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime6 - nTime5), nTimeCallbacks * MICRO, nTimeCallbacks * MILLI / nBlocksTotal);
1978 
1979+    TRACE7(validation, block_connected,
1980+        block.GetHash().ToString().c_str(),

jb55 commented at 1:14 pm on May 25, 2021:

It’s probably fine in this case, but we should avoid passing formatted data to USDTs when we can format it on the bpf side for performance reasons, especially in tight loops. It looks like we’re already passing GetHash().data(), can we format this on the ebpf side? I remember having trouble doing this in bpftrace, but perhaps that’s a temporary issue until bpftrace gets better buffer formatting.

0xB10C commented at 11:03 am on May 26, 2021:

Agree, ideally we’d only pass the GetHash().data() as first argument.

It looks like we’re already passing GetHash().data(), can we format this on the ebpf side? I remember having trouble doing this in bpftrace, but perhaps that’s a temporary issue until bpftrace gets better buffer formatting.

Right, the current %r printf formatting for buffers in bpftrace isn’t suitable for printing header hashes and txids. Adding a new formatting function to bfptrace similar to this as added in https://github.com/iovisor/bpftrace/pull/1107 doesn’t look to hard. However, the block hashes and txids would still be in little-endian.

in contrib/tracing/log_raw_p2p_msgs.py:70 in a076eb6dc2 outdated

65+// an BPF-array with a length of 1 to allocate up to 32768 bytes (this is
66+// defined by PCPU_MIN_UNIT_SIZE in include/linux/percpu.h in the Linux kernel).
67+// Also see https://github.com/iovisor/bcc/issues/2306
68+BPF_ARRAY(msg_arr, struct p2p_message, 1);
69+
70+// Two BPF perf buffers for pushing data (here P2P messages) to user -pace.

rajarshimaitra commented at 6:23 am on May 26, 2021:

nit: user space at the end.

in contrib/tracing/README.md:8 in a076eb6dc2 outdated

0@@ -0,0 +1,241 @@
1+Example scripts for User-space, Statically Defined Tracing (USDT)
2+=================================================================
3+
4+This directory contains scripts showcasing User-space, Statically Defined
5+Tracing (USDT) support for Bitcoin Core on Linux using. For more information on
6+USDT support in Bitcoin Core see the [USDT documentation].
7+
8+[USDT documentation]: ../doc/tracing.md

rajarshimaitra commented at 6:27 am on May 26, 2021:

I think this should be ../../doc/tracing.md.

in contrib/tracing/log_raw_p2p_msgs.py:83 in a076eb6dc2 outdated

78+    bpf_trace_printk("inbound!\\n");
79+
80+    // lookup() might not set the pointer `msg` to NULL. Thus, the BPF-verifier
81+    // requires that we check that `msg` is a NULL pointer.
82+    if (msg == NULL) {
83+        bpf_trace_printk("not NULL\\n");

rajarshimaitra commented at 7:41 am on May 26, 2021:

Little confused here, are we returning on NULL or not NULL?

0xB10C commented at 10:39 am on June 7, 2021:

Thanks! That comment was incorrect and super confusing. I’ve changed it to

0// lookup() does not return a NULL pointer. However, the BPF verifier
1// requires an explicit check that that the `msg` pointer isn't a NULL
2// pointer. See https://github.com/iovisor/bcc/issues/2595
3if (msg == NULL) return 1;

in doc/tracing.md:261 in a076eb6dc2 outdated

253+
254+[Installing BCC]: https://github.com/iovisor/bcc/blob/master/INSTALL.md
255+[`tplist` usage demonstration]: https://github.com/iovisor/bcc/blob/master/tools/tplist_example.txt
256+
257+```
258+$ tplist -l ./src/bitcoind -v

rajarshimaitra commented at 12:53 pm on May 26, 2021:

It seems to me the tplist tool binary name is tplist-bpfcc. Thats worked for me and its also mentioned like this in the bcc doc

0xB10C commented at 11:01 am on June 7, 2021:

Hm, I agree that this is the case for Ubuntu. There seem to be naming differences between distros.

I’ve documented that there might be binary naming differences depending on distro used and added tplist-bpfcc as an example for Ubuntu (Ubuntu is the most commonly used Linux distro, but I’m not sure how that relates to Bitcoin Core developers).

in contrib/tracing/log_raw_p2p_msgs.py:17 in a076eb6dc2 outdated

12+# submits the P2P messages to this script via a BPF ring buffer. The submitted
13+# messages are printed.
14+
15+# eBPF Limitations:
16+#
17+# Bitcoin P2P messages can be larger than 32kb (e.g tx, block, ...). The eBFP

jnewbery commented at 2:19 pm on May 26, 2021:

0# Bitcoin P2P messages can be larger than 32kb (e.g tx, block, ...). The eBPF

jonatack commented at 2:40 pm on May 26, 2021:

(there are 3 other s/eBFP/eBPF/ elsewhere in this doc to fix up too)

s/e.g/e.g./

in contrib/tracing/README.md:60 in a076eb6dc2 outdated

55+results in the port being cut off during logging. The string size limit can be
56+increased with the `BPFTRACE_STRLEN` environment variable (`BPFTRACE_STRLEN=70`
57+works fine).
58+
59+```
60+$ bpftrace contrib/tracing/log_p2p_traffic.bt

rajarshimaitra commented at 2:40 pm on May 26, 2021:

I am getting the following error here

0$ sudo bpftrace ./contrib/tracing/log_p2p_traffic.bt
1terminate called after throwing an instance of 'std::logic_error'
2  what():  basic_string::_M_construct null not valid
3Aborted

trace points seem to be defined. Node is running and talking to peers in testnet

Not sure what’s throwing this error here.

0xB10C commented at 11:02 am on June 7, 2021:

Thanks for reporting, I’ll have a look. Whats your bpftrace version (bpftrace --v)?

laanwj commented at 1:16 pm on July 27, 2021:

How is this supposed to find the process to attach to? Or would it log metrics for any bitcoind instance running on the system? (it seems so!) Unless you specify -p, I guess.

0xB10C commented at 3:10 pm on July 27, 2021:

The bpftrace scripts include a relative path to the bitcoind binary in ./src/bitcoind e.g. usdt:./src/bitcoind:validation:block_connected.

This is mentioned on the examples page.

jb55 commented at 5:01 pm on July 27, 2021:

I think something more general would be usdt:*bitcoind:net:inbound_message and then

bpftrace -p $(pgrep bitcoind) log_p2p_traffic.bt

eg this uprobe:

0sudo bpftrace -p $(pgrep bitcoind) -e '
1  uprobe:*bitcoind:"PeerManager::ProcessMessages(CNode*, std::atomic<bool>&)" { printf("node %p %d\n", arg1, *arg2) }
2'

seems to work for me…

jb55 commented at 5:37 pm on July 27, 2021:

oh hmm I don’t think you need *bitcoind, this also worked for me?

0sudo bpftrace -p $(pgrep bitcoind) -e '
1  uprobe:"PeerManager::ProcessMessages(CNode*, std::atomic<bool>&)" { printf("node %p %d\n", arg1, *arg2) }
2'

in contrib/tracing/README.md:85 in a076eb6dc2 outdated

80+Inbound and outbound traffic is listed for each peer together with information
81+about the connection. Peers can be selected individually to view recent P2P
82+messages.
83+
84+```
85+$ python3 contrib/tracing/p2p_monitor.py ./src/bitcoind

rajarshimaitra commented at 3:56 pm on May 26, 2021:

This is so cool. This is a communication sequence for a Peer asking for BIP157 filters.

It would be even super awesome if I could also select each message and can get the content.

0xB10C commented at 11:07 am on June 7, 2021:

It would be even super awesome if I could also select each message and can get the content.

I agree, however I like to keep this example slim. The idea is to provide a showcase and a code example for USDT. Advanced USDT scripts can be collected somewhere else.

in contrib/tracing/connectblock_benchmark.bt:138 in a076eb6dc2 outdated

133+  }
134+}
135+
136+END
137+{
138+  print("\nHistogram of block connection times in milliseconds (ms).");

Emzy commented at 4:28 pm on May 26, 2021:

I get the error: contrib/tracing/connectblock_benchmark.bt:138:3-71: ERROR: print() expects a map to be provided changing it from print to printf fixes it for me.

0xB10C commented at 4:51 pm on May 26, 2021:

Interesting.. Which version of bpftrace do you have installed? (bpftrace --v)

Emzy commented at 4:56 pm on May 26, 2021:

bpftrace v0.9.4 from Ubuntu 20.04.2 LTS

0xB10C commented at 9:39 am on June 7, 2021:

Seems like histogram support for print() was added in a later version of bpftrace (I’ve been using v0.11.4). I’ve changed it to printf() which should work with v0.9 and v0.11.

in contrib/tracing/p2p_monitor.py:221 in a076eb6dc2 outdated

216+        peer = peers[peer_id]
217+        screen.addstr(i + 4, 0,
218+                      row_format % (peer.id, peer.total_outbound_msgs, peer.total_outbound_bytes,
219+                                    peer.total_inbound_msgs, peer.total_inbound_bytes,
220+                                    peer.connection_type, peer.address),
221+                      curses.A_NORMAL if i + scroll == cur_list_pos else curses.A_DIM)

Emzy commented at 4:47 pm on May 26, 2021:

The curces.A_DIM vs A_NORMAL is not showing up in my terminal. I think this looks much better:

0curses.A_REVERSE if i + scroll == cur_list_pos else curses.A_NORMAL)

rajarshimaitra commented at 6:03 pm on May 26, 2021: contributor

Overall Concept ACK.

I have tested and ran the examples.

Below are some nits and bumps I faced.

Overall I think having some standard tracing points and readily available processing scripts can be very helpful for developers.

I liked the p2p message monitor, and excited to see what other tracing usage others come up with.

jamesob commented at 3:43 am on May 29, 2021: member

Concept ACK - very cool. Will review soon.

0xB10C force-pushed on Jun 7, 2021

0xB10C force-pushed on Jun 9, 2021

0xB10C commented at 12:50 pm on June 11, 2021: contributor

I’m marking this as ready for review. Added a commit documenting the systemtap dependency for USDT and installation instructions for Debian-like and Fedora (however, didn’t have a chance to test building and hooking into the tracepoints on these distros yet).

The macOS CI seems to complain about the macros in trace.h. Haven’t figured out what the issue is here. Would appreciate it if someone running macOS could try and build bitcoind with this patch.

0xB10C marked this as ready for review on Jun 11, 2021

fanquake commented at 5:50 am on June 14, 2021: member

The macOS CI seems to complain about the macros in trace.h. Haven’t figured out what the issue is here. Would appreciate it if someone running macOS could try and build bitcoind with this patch.

This should be solved by #22238.

fanquake referenced this in commit ad0c8f356e on Jun 18, 2021

fanquake commented at 7:18 am on June 18, 2021: member

@0xB10C if you rebase this now, the CI should be fixed.

0xB10C force-pushed on Jun 18, 2021

0xB10C commented at 8:48 am on June 18, 2021: contributor

@fanquake thank you! rebased.

theStack commented at 2:22 pm on June 18, 2021: contributor

Concept ACK. Very nice work, looking forward to reviewing and testing this in detail.

sidhujag referenced this in commit 0252cd649a on Jun 18, 2021

jb55 commented at 8:09 pm on June 24, 2021: contributor

Tested ACK 462a980ebc970da1da6f76af6db5d173cd9cf951, amazing work and great docs

DrahtBot commented at 7:07 am on July 8, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22412 by luke-jr

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in doc/dependencies.md:43 in 70f027341b outdated

39@@ -39,6 +40,7 @@ Some dependencies are not needed in all configurations. The following are some f
40 * SQLite is not needed with `--disable-wallet` or `--without-sqlite`.
41 * Qt is not needed with `--without-gui`.
42 * If the qrencode dependency is absent, QR support won't be added. To force an error when that happens, pass `--with-qrencode`.
43+* If the systemtap dependency is absent, USDT support won't compiled in. To force an error when that happens, pass `--enable-ebpf`.

theStack commented at 5:07 pm on July 9, 2021:

Is the error supposed to appear during the ./configure run? In my case, explicitely passing --enable-ebpf didn’t throw an error when systemtap-sdt-dev was still missing, only ebpf tracing = no was displayed on the summary at the end. Tested on Debian bullseye/sid.

0xB10C commented at 3:15 pm on July 27, 2021:

Thanks. I’ve tested this and the docs doesn’t seem to be correct. I think assumed this to be the case similar to the --with-qrencode behavior in the line above.

laanwj commented at 1:18 pm on July 27, 2021: member

~~Tested ACK 462a980ebc970da1da6f76af6db5d173cd9cf951~~ re-ACK 8f37f5c2a562c38c83fc40234ade9c301fc4e685

Edit: FWIW, I needed to sudo apt install python3-bpfcc to get the Python samples to work on Ubuntu 20.04.

doc: Add initial USDT documentation

Both added files are extended in the following commits.

doc/usdt.md is based on earlier work by laanwj.

Co-authored-by: W. J. van der Laan <laanwj@protonmail.com>

84ace9aef1

doc: document systemtap dependency 469b71ae62

tracing: Tracepoints for in- and outbound P2P msgs

Can be used to monitor in- and outbound node traffic.

Based on ealier work by jb55.

Co-authored-by: William Casarin <jb55@jb55.com>

4224dec22b

tracing: Tracepoint for connected blocks

Can, for example, be used to benchmark block connections.

8f37f5c2a5

0xB10C force-pushed on Jul 27, 2021

0xB10C commented at 3:20 pm on July 27, 2021: contributor

Sorry for invalidating the two ACKs. Addressed #22006 (review) and fixed another occurrence of #22006 (review) I previously forgot (and rebased on top of latest master).

git range-diff 462a980eb...8f37f5c2a

jb55 commented at 3:25 pm on July 27, 2021: contributor

range-diff LGTM!

ACK 8f37f5c2a562c38c83fc40234ade9c301fc4e685

laanwj added the label Needs release note on Jul 27, 2021

laanwj merged this on Jul 27, 2021

laanwj closed this on Jul 27, 2021

0xB10C deleted the branch on Jul 27, 2021

in src/net.cpp:3021 in 8f37f5c2a5

3017@@ -3017,11 +3018,20 @@ bool CConnman::NodeFullyConnected(const CNode* pnode)
3018 void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
3019 {
3020     size_t nMessageSize = msg.data.size();
3021-    LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n",  SanitizeString(msg.m_type), nMessageSize, pnode->GetId());
3022+    LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n", msg.m_type, nMessageSize, pnode->GetId());

MarcoFalke commented at 12:10 pm on July 28, 2021:

why is this refactor(?) mixed in a non-refactor commit without any explanation?

jnewbery commented at 12:24 pm on July 28, 2021:

See #22006 (review)

MarcoFalke commented at 12:29 pm on July 28, 2021:

It would have been good to split the refactor from the feature commit or at least mention it in the commit body. Otherwise reviewing locally is impossible and one needs to rely on GitHub, which showed in the past to be corrupting both code and metadata or be unreachable.

0xB10C commented at 12:29 pm on July 28, 2021:

Didn’t consider that this needs a explanation in the commit message.

0xB10C commented at 12:31 pm on July 28, 2021:

It would have been good to split the refactor from the feature commit or at least mention it in the commit body. Otherwise reviewing locally is impossible and one needs to rely on GitHub, which showed in the past to be corrupting both code and metadata or be unreachable.

Yes, makes sense.

sidhujag referenced this in commit 1f479d9c1a on Jul 28, 2021

0xB10C referenced this in commit bc771fd41b on Oct 18, 2021

0xB10C referenced this in commit 82ad1c4bdd on Oct 18, 2021

0xB10C referenced this in commit 11d4406f5d on Oct 18, 2021

0xB10C referenced this in commit 604ca386bb on Oct 18, 2021

0xB10C referenced this in commit 53c9fa9e62 on Oct 18, 2021

fanquake referenced this in commit 4b24f6bbb5 on Oct 19, 2021

sidhujag referenced this in commit 230bd8aef6 on Oct 19, 2021

laanwj referenced this in commit 913b7148a2 on Nov 29, 2021

rob-scheepens commented at 2:26 pm on March 18, 2022: none

As BPF CO-RE arguably is the future direction for BPF, it’s perhaps interesting to explore using libbpf/CO-RE instead of BCC. See https://nakryiko.com/posts/bpf-portability-and-co-re/#bpf-co-re-as-of-2021 and https://nakryiko.com/posts/bcc-to-libbpf-howto-guide/#bpf-code-conversion.

jb55 commented at 3:22 pm on March 18, 2022: contributor

On Fri, Mar 18, 2022 at 07:27:11AM -0700, rob-scheepens wrote:

As BPF CO-RE arguably is the future direction for BPF, it’s perhaps interesting to explore using libbpf/CO-RE instead of BCC. See https://nakryiko.com/posts/bpf-portability-and-co-re/#bpf-co-re-as-of-2021 and https://nakryiko.com/posts/bcc-to-libbpf-howto-guide/#bpf-code-conversion.

This looks great! It might be a fun exercise to convert some of our bcc tools to this for anyone wanting to get more into bitcoin tracing.

0xB10C commented at 3:48 pm on March 18, 2022: contributor

@rob-scheepens agree that BPF CO-RE and libbpf is the way to go for future tracing programs. I’ve looked into this a while ago, but it seems libbpf doesn’t support USDT tracepoints yet (see https://github.com/iovisor/bcc/issues/3007). However, there was a mention of a soon to be open sourced libusdt library developed and used in production at Meta by @anakryiko here. Looking forward to testing this once available!

anakryiko commented at 8:52 pm on March 21, 2022: none

I’m actually cleaning up the code a bit and preparing patches for submission to include proper USDT support in libbpf itself. So be on the lookout for upstream patches this week (or at worst next week, if I get distracted with other stuff this week).

laanwj referenced this in commit 6c9460edae on Apr 6, 2022

anakryiko commented at 6:47 pm on August 15, 2022: none

I’m actually cleaning up the code a bit and preparing patches for submission to include proper USDT support in libbpf itself. So be on the lookout for upstream patches this week (or at worst next week, if I get distracted with other stuff this week).

BTW, USDT support was added into libbpf a while ago. See bpf_program__attach_usdt() API and SEC(“usdt”) examples in kernel selftests/bpf. libbpf-bootstrap also has USDT example (https://github.com/libbpf/libbpf-bootstrap/blob/master/examples/c/usdt.c and https://github.com/libbpf/libbpf-bootstrap/blob/master/examples/c/usdt.bpf.c).

fanquake removed the label Needs release note on Sep 15, 2022

fanquake commented at 3:15 pm on September 15, 2022: member

This change was part of 23.0, and had a release note, so removing “Needs release note”.

Fabcien referenced this in commit abafc354a0 on Nov 25, 2022

gwillen referenced this in commit 4334fce793 on Mar 28, 2023

bitcoin locked this on Sep 15, 2023

tracing: first tracepoints and documentation on User-Space, Statically Defined Tracing (USDT) #22006

Context

USDT? Stablecoin?

Open Questions (Not in scope for this PR)

Todo (before undraft)

Conflicts