GGraziadei commented on PR #9154: URL: https://github.com/apache/storm/pull/9154#issuecomment-6045359050
Alert: comment refactored with an LLM (GPT 5.6) Thanks for the latency numbers and the BSP sweep, @dpol1. They help a lot. Reading through the hot path, my main concern is the cost at ratio 0: about -29% throughput and +0.7 ms at p99. This cost is paid by every topology that enables the tracer, even when nothing is sampled. I think most of this can go away with a few small changes while keeping the SPI as it is. For unsampled trees, we should avoid calling the tracer altogether. With a parent-based sampler, no child of an unsampled root can ever be sampled. We could keep the root's sampled flag on the tuple (`TupleImpl`/`TupleInfo`) and have `BoltExecutor`, `BoltOutputCollectorImpl` and `SpoutExecutor` skip tracing when it is false. Right now an unsampled tuple still goes through `startExecute`, `boltEmit` and `spoutOutcome`, and with the agent each of those calls crosses the API bridge and allocates wrappers. The cost should ideally become just a branch. We could also let Storm make the sampling decision at the spout. A `topology.tracing.sample.ratio` check in `SpoutOutputCollectorImpl.sendSpoutMsg` would mean the tracer is only called for trees that are actually selected. At ratio 0, the SDK would never be touched. The existing `rootId` from `MessageId.generateId(random)` gives us randomness for acked trees, with `random` as a fallback for unanchored emits. Using it to derive the trace ID would also make it possible to correlate Storm debug logs with traces. The SDK sampler could then stay parent-based (or always-on), and we could document this in `Tracing.md`. We should also avoid encoding and decoding unsampled contexts. `TraceContextCodec.encode` currently writes 26+ bytes for every cross-worker tuple, and `decode` allocates several objects even when the tuple is unsampled. Once the fast path is in place, this will mostly happen only for sampled tuples. As a cheap safety net, `decode` could read the flags byte first and return null when the sampled bit is clear, before doing any allocations. So the changes should be limited to `BoltExecutor`, `BoltOutputCollectorImpl`, `SpoutOutputCollectorImpl`, `KryoTupleSerializer`/`Deserializer` and the tracer. Could you implement these changes and re-measure ratio 0 and 1% on the current head? The existing throughput numbers are from before the SPI split. For ratio 0, I would expect performance to be within the noise of "agent, tracing off". That is the key property we need before recommending that tracing can be enabled by default. A couple of measurement requests are not blocking: - 50k tuples/s is only about 10% of the measured capacity, so the latency numbers show the added service time but not the queueing effect from the lost throughput. It would be useful to add one or two higher-load points, such as 50% and 75% of the tracing-off maximum, with runs of a few minutes. Discard the first minute and build the histogram from the rest. This should make the p99 differences easier to see. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
