[Core] Support for sent notification for unary messages #3213
Conversation
346b420 to
7fa13e4
Compare
There was a problem hiding this comment.
Thanks for creating this PR @AhmedSoliman. The changes look good to me. I only had a question regarding the motivation for removing the originating span for unary calls. Couldn't it be helpful for tracing where messages came from?
| #[cfg(feature = "test-util")] | ||
| header: custom_header, | ||
| })) => { | ||
| let mut header = Header::default(); | ||
| self.fill_header(&mut header, span); | ||
| self.fill_header(&mut header, None); |
There was a problem hiding this comment.
Are we primarily not sending the originating span to save some bytes? Are those the 8 bytes that we are saving? Couldn't it be helpful for tracing when trying to understand where messages came from?
There was a problem hiding this comment.
In fact, at the moment distributed tracing for the message fabric is disabled, but when we enable it back, I believe that the majority of the value will come from tracing RPC operation rather than unary messages. Unary messages are meant to be lightweight and fast (fire-and-forget).
In addition, this is not a one way decision, it doesn't break compatibility to add this back in a future version if we want.
It's worth noting that the saved bytes also come from swapping the String with the &'static str.
Other changes: - Smaller EgressMessages by using static string for message names, removing span for unary messages (224 bytes -> 216 bytes) - ReplyRx doesn't require protocol version at creation time, this will become relevant in a subsequent PR. The protocol version is now returned along with the reply when it arrives
Other changes:
Stack created with Sapling. Best reviewed with ReviewStack.