feat(symbolizer): expose address-level symbol resolution as a public API#5317
feat(symbolizer): expose address-level symbol resolution as a public API#5317jake-kramer wants to merge 7 commits into
Conversation
81c0e27 to
29f06e1
Compare
|
This change is part of the following stack:
Change managed by git-spice. |
…thout functions
The compaction symbols rewriter replaced every location of a partition
whose functions table is empty with a zero value: the branch that drops
line records for such partitions also skipped storing the location,
leaving InMemoryLocation{} in the lookup table. Partitions like this
are exactly the native, not-yet-symbolized profiles, where every
location is line-less, so compaction permanently destroyed the
addresses needed to symbolize them at read time — the source blocks
are deleted after compaction, so the loss is unrecoverable. Both the
v1 and v2 compactors rewrite symbols through this path.
Keep dropping the line records, which cannot reference any function,
but store the location so its address and mapping survive.
Introduced in #4051.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The symbolization end-to-end test asserted only the first successful query, which happens before the pushed segment is compacted, so the compaction rewrite of the block symbols was never exercised. Extend the test past the first L0 compaction: filler blocks drive the age-based batch flush, which is evaluated only when another block arrives at the level, and the post-compaction assertions check the symbolized frames without depending on location order, which compaction may change. Without the rewriter fix, this fails with libfoo.so!0x0 fallback frames as soon as the segment is compacted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a Resolver interface and a Resolve(ctx, buildID, binaryName, addrs) method that pulls per-address symbol lookup out from behind the pprof-only entry point. It gives future consumers a narrow interface to depend on directly, without detouring through a synthetic pprof profile just to get symbols resolved -- groundwork for moving tree-shaped query symbolization out of the query rewrite. SymbolizePprof now calls Resolve internally, and its output is unchanged byte-for-byte on every existing fixture. Two deliberate behavior notes: - Resolve no longer synthesizes the "binary!0xaddr" fallback text itself. A nil/empty result slot means "unresolved," and rendering the fallback is now the caller's job: SymbolizePprof's per-mapping goroutine does it immediately after Resolve returns, so nothing downstream sees a difference. - Resolve returns a non-nil error only when the context is done during resolution. That now surfaces as a real error out of SymbolizePprof, instead of silently returning a fallback-filled profile with a nil error. This is the one externally-visible behavior change here, and it only affects a path no existing fixture exercised. Resolve also independently validates buildID with the same sanitizer extractBuildID already applies, before doing any object-store or debuginfod I/O. The only caller today already sanitizes first, so this is a no-op in practice, but it's defense in depth now that Resolve is a public entry point a future caller could reach directly with unsanitized input. Part of grafana/pyroscope-squad#487. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
29f06e1 to
436c24e
Compare
simonswine
left a comment
There was a problem hiding this comment.
While I think the error propagation is great, I am not too sure that is better to fail the whole flamegraph query, when a single symbolisation fails.
I think we should retain the user observable behaviour of using fallback symbols if we couldn't resolve some of the symbols.
The debuginfod fetch flight is shared by every concurrent caller for a build ID, but ran on the initiating caller's context, so one caller's cancellation failed all waiters. Run the flight on a context detached from the caller (values preserved, cancellation dropped); the HTTP client timeout and bounded retries keep it from running unbounded. Correspondingly, Resolve no longer classifies context errors propagated from the fetch layer as its own: only the caller's context ending fails the call, and every other failure - including another caller's cancellation surfacing through a shared flight - degrades to unresolved slots, which render as fallback symbols. Suggested by simonswine in review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… fallbacks Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e054fad. Configure here.
…od fetch Detaching the deduplicated debuginfod fetch from caller cancellation had cut callers loose in both directions: a caller canceled mid-fetch could still receive a successful result, and a canceled caller had to wait out the whole detached fetch (HTTP timeout times retries) before returning. FetchDebuginfo now waits on the shared result and the caller's context, whichever ends first, and its doc comment owns the error contract: a done caller gets its own context error immediately; any other error comes from the shared fetch and says nothing about the caller's own context. Resolve folds its post-fetch checks into a single own-context check instead of explaining client internals at the call site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e054fad to
441abfe
Compare
simonswine
left a comment
There was a problem hiding this comment.
Thanks for fixing the context cancellation and looking into the fallback behaviour

Part of a series: symbol-aware tree references (1 of 8)
The full picture. Native and eBPF profiles are ingested with raw memory addresses; function
names are resolved at query time from debug info. Today that resolution only triggers when the
query-frontend sees an
__unsymbolized__dataset label — which queryless queries ({}) neversurface, so their results silently lose symbolization — and it works by rewriting the whole TREE
query into PPROF and back, which makes every dataset in the query pay the pprof cost and silently
drops any query selector that isn't hand-mirrored between the two query types. This series changes
the tree representation instead: tree nodes can reference unresolved
{buildID, address}locationsdirectly, partial trees carry those references through every merge (with truncation deferred), and
the frontend resolves them once after the final merge, then rewrites and truncates the tree.
Correct for every query shape, no pprof detour, and the wire format stays a tree. Tracking issue:
grafana/pyroscope-squad#487.
The series, in order (each PR stands alone for review; no new behavior activates until 6):
ResolveAPI ← this PR.Extracts per-address symbol lookup from behind
SymbolizePprofinto a publicResolverinterface that later PRs consume directly. Pure refactor: no caller change, no new behavior.
SymbolRefTable+ tree query/report fields.The wire schema for trees that carry unresolved references — a compact side table of resolved
names, interned build IDs, and addresses. Schema + generated code only; no consumers yet.
model/symbolref: intern table + merge rebasing.New package implementing the reference table and deterministic reference rebasing when partial
trees from different backends merge.
model/symbolref: job grouping + rebuild.Groups unresolved references into per-build-ID resolution jobs, and rebuilds the final resolved
tree — expanding inline-frame chains, merging newly-identical nodes, truncating exactly once.
Backends emit trees with unresolved references for unsymbolized datasets (deferring truncation)
and merge partials of both shapes; fully-symbolized datasets keep today's exact path.
The frontend requests symbol-ref trees, resolves post-merge via this PR's API, and rewrites the
result. The first PR where any new behavior can be switched on.
End-to-end coverage — including the queryless-query case that motivated the issue — plus an
equivalence test pinning the new path's output against the legacy one.
Fixes a pre-existing bug (fix: drop malformed locations #4051) in the shared compaction symbols rewriter that replaced every
location of a partition without functions — exactly the fully-unsymbolized native datasets this
series serves — with a zero value, permanently destroying the addresses symbolization needs.
Found during end-to-end validation of this series; stacked at the bottom so it lands before the
feature can be enabled anywhere.
What / Why
pkg/symbolizercould previously only be driven throughSymbolizePprof, which takes a wholepprof profile. Anything that wants symbols for a set of
{buildID, address}pairs — for example,resolving symbols for a tree-shaped query result — had no way to ask for that directly; it would
have had to synthesize a throwaway pprof profile just to reuse the lookup path.
This PR extracts that lookup into a narrow, exported interface:
SymbolizePprofis refactored to callResolveinternally instead of duplicating the lookuplogic.
Resolveis defined on*Symbolizerin the producer package (pkg/symbolizer), the samepattern
pkg/objstore.Bucketalready uses, so a later out-of-process implementation can satisfyResolverwithout importing this package.What moved: fallback-symbol synthesis (the
binary!0xaddrplaceholder text).Resolveitselfnever produces it — a nil/empty slot in its result means "could not resolve this address," full
stop. Rendering the placeholder is now the caller's job:
SymbolizePprof's per-mapping goroutinecalls the existing
createFallbackSymbolhelper immediately afterResolvereturns, for every nilslot. This is a mechanical relocation of an existing call, not a change to what it computes.
What stayed byte-identical:
SymbolizePprof's emitted profile, on every existing fixture — same metric names/label values,same log message text, same check order as the code this replaces.
(
var framesBuf []lidia.SourceInfoFrameis never reassigned, so eachtable.Lookupcall getsnil) — preserved exactly as-is; not something this PR changes or "fixes."Behavior changes
There is exactly one externally-visible behavior change:
Concretely:
Resolvereturns a non-nil error only whenctx.Err() != nil(checked at entryand again around the debug-info fetch, via
errors.Isagainstcontext.Canceled/context.DeadlineExceeded); every other failure mode (empty/invalid buildID, fetch failure,malformed debug info, a specific address not resolving) keeps degrading to a nil/empty result slot
with a
nilerror, exactly like the code it replaces.This matters today at the three real call sites that invoke
SymbolizePprof, all of which alreadytreat any error it returns as fatal to the request:
pkg/frontend/readpath/queryfrontend/symbolizer.go:82pkg/frontend/readpath/queryfrontend/query_select_merge_profile.go:112pkg/frontend/readpath/queryfrontend/query_select_merge_profile.go:237Before this PR, a query whose context was canceled or timed out mid-symbolization would silently
get back a profile full of
binary!0xaddrplaceholders and no error. After this PR, that samerequest now fails with a wrapped
context.Canceled/context.DeadlineExceedederror, matching howthese call sites already handle every other symbolization failure.
Internal contract note (not observable from outside
pkg/symbolizer, sinceSymbolizePprof'soutput is unaffected): whenever
Resolvereturns anilerror, the returned outer slice always haslen(result) == len(addrs), andresult[i] == nilmeans addressididn't resolve, for any reasonshort of context cancellation.
Security hardening
Resolveis a new public entry point into a buildID-driven object-store path(
getLidiaBytes→fetchLidiaFromObjectStore→path.Join(bucketPrefix, tenantID, buildID)).Previously, this path was only reachable through
SymbolizePprof, whose one caller alwayssanitized
buildIDfirst — so encapsulation, not validation, was what kept it safe. Now thatResolveis exported,ResolvevalidatesbuildIDitself (same sanitizerextractBuildIDalreadyapplies) before doing any object-store or debuginfod I/O, and degrades to a nil-per-miss result
instead of trusting the caller. A test asserts zero mock calls on the storage/debuginfod clients
when
buildIDfails validation, proving the rejection happens before any I/O — the property thatactually matters for a future caller that might source
buildIDfrom less-trusted input.Testing
pkg/symbolizersuite passes under-race, including the pre-existing golden-baselinetests (
TestSymbolizePprof,TestSymbolizationKeepsSequentialFunctionIDs) with zero edits toany existing assertion.
path).
(deterministic, channel-coordinated, no sleeps) — plus a third subtest for
context.DeadlineExceededvia a real timeout.SymbolizePprofend-to-end.(
MaxDebuginfodConcurrency = 4), with a blocking-mock/channel handshake that forces genuine3-way overlap (a regression to serialized execution hangs the test rather than passing).
debuginfod client or bucket, plus the new
invalid_build_idmetric label incrementing exactlyonce.
pkg/symbolizer.go build ./...andmake lint(golangci-lint run ./...) are both clean across the whole repo,not just this package.
Follow-ups
None deferred from review.
Resolve's two adjacentstringparameters (buildID,binaryName)were flagged as a general API-ergonomics observation during review and deliberately left as-is —
the signature is pinned across the rest of this series and the one call site already threads both
arguments correctly.