RFC-0010: Shared-memory buffer (SMB) scraping for remote producers

[github-actions[bot]]

📄 RFC Doc: 0010-smb-scraping-remote-producers.md

---

Shared-memory buffer (SMB) scraping for remote producers

Authors: @jahdiel-alvarez

Status: Decided

Problem

Shared memory buffer (SMB) scraping is a mechanism in the tracing service which
finds and copies any uncommitted chunks from a producer’s SMB into the central
buffer. There are multiple scenarios during a tracing session where a producer
might still have uncommitted chunks in their SMB, therefore in order to not lose
the data the tracing service needs to retrieve it from the buffer. Some of these
scenarios are when a producer disconnects, when a Flush completes or when a
tracing session ends.

A gap in the current implementation is that this isn’t supported for remote
producers (connected over traced_relay). This means that remote producers with
TrackEvents or any other data source that isn’t explicitly calling Flush at the
end of a trace session will possibly end up with trace data inside an uncommitted
chunk that will never be sent to the tracing service.

Decision

After various iterations we settled on the following design:

Intercept OnFlush calls in the remote producers

Instead of adding a new method to the current tracing protocol between traced and
producers, we can intercept the OnFlush service requests coming
from the service at the producer IPC client level. When the requests are
intercepted, the producer should verify if it is in SMB emulation mode and if
it is then it will go ahead and do internal SMB scraping. Once done, the process
will commit the uncommitted chunks to the tracing service.

Force flushing remote producers

The current implementation of flush service requests in the tracing service only
sends a flush request if at least one of the producer's data sources overrides
the onFlush method. The design will be updated such that if the tracing service
is sending out a flush request and it is dealing with a remote producer, the flush
IPC call will be made to the producer regardless of whether any data source
supports the flush request. This IPC calls (as mentioned above) will then be
intercepted at the producer IPC level and will trigger the internal SMB scraping
workflow.

Support incomplete chunks in CommitDataRequest IPC protocol

When calling the CommitData RPC method from the producer we need to add a new
boolean parameter to CommitDataRequest::ChunksToMove to specify whether the chunk
being sent is complete or incomplete. The current implementation assumes all chunks
sent over IPC are complete and this has two effects:

1. The tracing service will try to read the last packet in the chunk which could be
   write-in-progress.
2. TraceBuffer will not allow a re-commit later of the chunk, once it is complete.

In terms of backwards compatibility, we don't need to do much. Older versions of the
service will ignore the new boolean parameter and treat the chunk as complete. This
would mean that in some rare cases you might hit some trace errors (flagged as data
losses). But given the chance is rare and the current implementation is already
lossing far more data without this proposal, it is a better choice and a safe change.

Alternatives considered

Option 1: Extend the tracing protocol

Another solution to this issue is to extend the SMB scraping method to remote
producers. The main logic of SMB scraping is in
TracingServiceImpl::ScrapeSharedMemoryBuffer, this method takes a producer and
copies the uncommitted chunks from the SMB into the central buffer. This method
can be extended so that when it is called for a remote producer, the tracing
service calls GetUncommittedChunks (a new IPC method) on the producer. This
call will then be handled by the tracing muxer, which will scrape the emulated
SMB and pass the data back to the tracing service. The returned data will be
handled in the exact same way as it is done for local producers.

Option 2: Intercept OnStop/OnFlush calls in the remote producers

Same approach as the final decision but included intercepting the OnStop service
request as well. It was decided against this approach because you should never see
OnStop without an OnFlush request (there are some exceptions like driving the
consumer protocol yourself). There is more risk to waste too much time by intercepting
both requests and missing the stop deadline imposed by the tracing service.

Note for Posterity

In theory, we should look on whether the producer is using shmem emulation. However,
use_shmem_emulation_ is only part of the handshake between producer and traced_relay,
TracingServiceImpl doesn't know anything about it. For now we will go with this
proposal, because currently there is a 1:1 mapping between "being a remote producer"
and "using shmem emulation". Technically speaking a local producer could choose to use
shmem emulation in which case the logic here would not be sufficient. Since there are
no known use-cases of this today, we accept it for the sake of pragmatism.

---

💬 Discussion Guidelines:

• This discussion is automatically synced with the RFC document
• Please provide constructive feedback and suggestions

[primiano]

Hey thanks for sending this out.
I have not looked at the PR yet (i'm going to do this now, wanted to look at this).

LGTM to this doc. Here are some comments (Can you add them to the rfc so they stay?), but feel free to move it to "Status: Decided"

we can intercept the OnStop and OnFlush

It's questionable whether you need OnStop at all. You should never see OnStop without an OnFlush (% some weird edge cases that were mostly bug, now fixed, of our cmdline. Or if you drive the consumer protocol yourself). IMHO there is more risk (by doing both) that we waste too much time and miss the stop deadline.

In my mind:

PRO: is not going to hurt too much in most cases. It will cover the very unlikely cases of a consumer that implements the tracing protocol (e.g. some tooling like gpuinspector.dev) and that forgets to do a flush before stop. Our cmdline client does it.

CON: there is a chance that we pile up more work while stopping and we might miss the stop timeline if there is CPU contention.

Let's decide once we have the impl and see how it works in practice.

the producer should verify if it is in SMB emulation mode and if it is then it will go ahead and do internal SMB scraping.

+1 this is the key. This was really an oversight of the SMB emulation code, and should be fixed at that layer.

if the tracing service is sending out a flush request and it is dealing with a remote producer,

(can you add the below to the RFC so stays in the records?)

Note for posterity: in theory we should look on whether the producer is using shmem emulation. However I just realized while looking at code that the use_shmem_emulation is only part of the handshake between producer and traced_relay, TracingServiceImpl doesn't seem to know anything about it. So for now we go with your proposal, because to the best of my knowledge there is a 1:1 mapping between "being a remote producer" and "Using shmem emulation".
Technically speaking a local producer could choose to use shmem emulation (in which case the logic here would not be sufficient). But there are no known use-cases of it today, so we accept it for the sake of pragmatism.

[jahdiel-alvarez]

Updated the RFC to reflect some of the comments here.

[github-actions[bot]]

📝 RFC Document Updated

View changes: Commit History

[primiano]

One thing we found out later: (can you plz update the RFC)

when doing commits over IPC we need to add a boolean to CommitDataRequest::ChunksToMove as we need to tell the service whether the chunk is incomplete or not. Today it will assume all chunks received over IPC are complete. THat has two effects:

1. the tracing service will try to read the last packet in the chunk which could be write-in-progress
2. TraceBuffer will not allow re-commit later (once the real complete chunk arrives)

There is a quesiton on: so how do we handle backward compatbility? The good news is that we don't need to do much:
older version of the service will ignore the new bit, and treat it as complte. which means that in some rare cases you might hit some trace errors (flagged as data losses).
But overall the chance is rare, and you have far more data losses now without this proposal. so it's a safe change.

[jahdiel-alvarez]

Updated the RFC to reflect this comment.

[github-actions[bot]]

📝 RFC Document Updated

View changes: Commit History