fix(profiler): capture nested RPC timings and emit valid speedscope output - #949
Merged
Kingsman-99 merged 2 commits intoSep 28, 2026
Merged
Conversation
Two real gaps against the issue's requirements: 1. rpcCalls was never populated. The report() code synthesised nested frames from entry.rpcCalls, but nothing ever pushed to that array, so 'nested RPC call timings' was dead code and no flame graph could show where time actually went. Wrap each client's Soroban RPC server on start() and restore it on stop(). Instrumenting the server rather than individual SDK call sites keeps this to one place, so new SDK methods are profiled for free. A per-invocation sink (restored on every exit path, including thrown errors) attributes each RPC to the SDK call that issued it, and prevents concurrent calls from cross-contaminating. RpcCallTiming gains a timestamp so report() can place nested frames at their real offsets rather than distributing them evenly, which made flame graphs misleading about ordering. 2. The speedscope output was missing the 'exporter' field required by the v0.6 schema, so profiles were rejected by speedscope itself. Surfaced by switching the test suite from a hand-rolled validator to real ajv validation against the published schema — the hand-rolled check only asserted the fields it happened to think of. Closes Stellar-split#849
|
@maztah1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What the issue was
#849 asked for a
ProfilerSessioncapturing timings for every SDK operation, withreport()producing speedscope v0.6 JSON,exportJSON(path), and "nested RPC call timings" as an explicit requirement. Validation "with ajv" was also an acceptance criterion.Most of it existed and worked. I verified against the acceptance criteria and found two things that were not actually true.
Gap 1 —
rpcCallswas never populatedreport()synthesised nested flame-graph frames fromentry.rpcCalls, but nothing ever pushed to that array. TheRpcCallTimingplumbing, therpc:frame naming, the nesting logic inreport()— all of it operated on a field that was alwaysundefined. So "nested RPC call timings" was dead code, and no flame graph could show where time inside an SDK call actually went.The fix wraps each client's Soroban RPC server on
start()and restores it onstop(). Instrumenting the server rather than individual SDK call sites keeps this to one place, so new SDK methods are profiled for free. Timings route through a per-invocation sink that is restored on every exit path — including thrown errors — which attributes each RPC to the call that issued it and stops concurrent SDK calls from cross-contaminating each other's timings.RpcCallTimingalso gained atimestamp, becausereport()was previously distributing nested RPC frames evenly across the parent window. That made flame graphs actively misleading about ordering. They're now placed at their real offsets, clamped to the parent bounds.Gap 2 — the output wasn't actually valid speedscope
Switching the test suite from a hand-rolled validator to real ajv validation (ajv is already a declared dependency — the previous code comment said "replaces ajv without requiring the package to be installed", which was unnecessary) immediately failed:
The speedscope v0.6 schema requires
exporter, andreport()never emitted it — so profiles were rejected by speedscope.app itself. The hand-rolled validator missed it because it only asserted the fields its author happened to think of. Fixed by emittingexporter: "@stellar-split/sdk".This is the clearest argument for the ajv criterion in the issue: the approximate check passed while the real schema check failed.
How it was tested
26 tests in
test/profiler.test.ts, up from 21:validateSpeedscopeSchemanow compiles the published v0.6 schema with ajv (kept inline so the suite stays hermetic and never fetches over the network).rpcCallsentry naming the operation with a non-negative duration; nestedrpc:frames appear in the report; the RPC server is restored onstop();report()validates against the schema and carriesexporter.One subtlety worth noting: the test stubs the RPC endpoint before
start(). My first attempt stubbed it inside the profiled method, which replaced the profiler's own wrapper and made capture silently fail — the RPC then made a real network call. The test now documents that ordering constraint.Full suite: 213 passed, 1 skipped, 0 failed.
tsc --noEmitdiffed against base — no new errors.Small client change
StellarSplitClient._instances(marked@internal) is a staticSetof live clients, populated in the constructor, so the profiler can find each client'sserver. Without it the profiler has no way to reach the per-instance RPC objects. Note the prototype patching that was already there remains unchanged — a separate global-patching concern I did not touch here, since it's outside this issue's scope.closes #849