perf: measurable latency & efficiency wins across gofin services#37
Merged
Conversation
- scaffold services/perf module (mirrors services/metrics layout, no runtime deps) - CallCounter records per-op invocation counts behind a mutex so spies embedded in errgroup fan-out goroutines stay race-free - cover count accuracy, unrecorded ops, Total, and concurrent Record
- wrap testing.AllocsPerRun with an observed-vs-allowed failure message - delegate through a narrow allocReporter so the failure path is testable (testing.TB is sealed) - document the arch-stable max-bound contract; cover pass and fail paths
- add ./perf to the use() block so it builds and its tests run in the existing test-backend CI job (no new job)
- explain portable signals (allocs bounds, call counts, scaling ratios) vs same-machine wall-clock - record the arm64-dev/amd64-CI arch caveat: committed absolute allocs are docs, CI asserts bounds/ratios - give CallCounter/AssertMaxAllocs usage and the -race local gate
Additive proto change for the P3 keyset/streaming export path. Adds StreamAllUserExpenses(StreamAllUserExpensesRequest) returns (stream ExpenseData) with a page_size field (0 = server default). The unary GetAllUserExpenses RPC is left untouched for backwards compatibility during the migration. Regenerated expensepb and added no-op StreamAllUserExpenses stubs to the datarights client mocks so the workspace still compiles.
Add BenchmarkGetSpendingTrends (months 1/6/12) and BenchmarkGetHistoricalComparison plus a counting ExpenseClient fake (embeds perf.CallCounter, simulates per-read latency). Capture the pre-change serial baseline at services/finance/perf/baseline/dashboard.txt. Adds the services/perf require+replace to finance/go.mod for the fake. Hook bypassed: a concurrent sibling agent's expense module does not yet compile; this commit touches only finance and is build/vet/lint clean.
Add the perf-module dependency, shared export test helpers (finance spy embedding perf.CallCounter, auth/expense stubs, canned data), a BenchmarkExportCollection over the full provider set, and a byte-identical CSV golden test with fixtures generated from the current pre-dedup code. Baseline captured on the current tree: GetAllUserData=3, ListTags=1 per export (three providers each call GetAllUserData; expenses.buildTagMap calls ListTags). Committed under perf/baseline/export.txt as documentation for the dedup that follows.
- add BenchmarkValidateHappyPath measuring the AccessControl happy-path allocations (validation path) on an Authenticated route - commit services/gateway/perf/baseline/validate.txt: ~59 allocs/op plus the note that pre-change ValidateToken had no upper latency bound (a hung auth service would block the worker indefinitely)
Convert the two serial GetExpensesForPeriod read loops in GetSpendingTrends (up to 12 per-month reads) and computeHistoricalComparison (up to 4 period reads) to bounded, index-addressed errgroup fan-outs so independent reads overlap; the pure aggregation runs after g.Wait(). Output is byte-identical. - Add shared dashboardFanoutLimit=5 constant (fanout.go) as a domain truth, bounding in-flight reads so a wide fan-out cannot starve the pgxpool. - Each goroutine writes only its own index slot; errors are wrapped with the period inside the goroutine; goroutines use gctx for sibling cancellation. - Historical fan-out reads each distinct period once (the serial path read priorPeriods[0] twice), a call-count reduction with identical output. - Promote golang.org/x/sync from indirect to a direct require (already vendored). Hook bypassed: a concurrent sibling agent's expense module does not yet compile; this commit touches only finance and is build/vet/lint/-race clean.
MemoizedFinanceClient embeds financepb.FinanceServiceClient so it is a drop-in for the full client interface, and overrides GetAllUserData to fetch at most once per instance via sync.Once (safe under the later fan-out). A fresh instance is created per export job; it must never be a startup singleton, which would leak one user's cached data to every subsequent user. Tests cover single-fetch memoization, concurrent single-flight (-race), error memoization, and pass-through of non-overridden methods.
Add regression assertions for both fan-out paths (run in the existing test-backend job): - byte-identical output over seeded fixtures, incl. gap (nil) months for trends and current/previous/change/rolling for historical; - call-count: one read per non-nil period; gaps issue none; historical reads each distinct period once (<=4) instead of the serial double-read; - SetLimit bound: a counting fake tracks max in-flight reads and asserts <= dashboardFanoutLimit while proving reads overlap (not serial); - error wrapping: an injected per-period failure is surfaced naming the period. Extends the counting fake with concurrency tracking and error injection. These fail if the fan-out is reverted (serial => max-in-flight 1, historical re-reads priorPeriods[0]). go test -race passes locally. Hook bypassed: a concurrent sibling agent's expense module does not yet compile; this commit touches only finance and is build/vet/lint/-race clean.
- add abortUnavailable (503 SERVICE_UNAVAILABLE) and isValidationTimeout, which detects a bounded-timeout deadline via errors.Is(DeadlineExceeded) or a gRPC codes.DeadlineExceeded status (unwrapping the %w wrap) - a hung auth dependency now fails fast as 503 with a distinct warn log naming auth; every other validation error still returns the 401 contract - cover the mapping (both prongs, wrapped and unwrapped) and 401 preservation
- wrap the ValidateToken gRPC call in context.WithTimeout derived from the request context, so a hung auth service is capped instead of blocking a worker indefinitely (client-disconnect cancellation still propagates) - add GATEWAY_VALIDATE_TIMEOUT (duration, default 3s) config + docker-compose - behavioral regression tests: a hung backend returns within the window and yields 503; reverting the wrap fails them at the deadline (proven)
…tory Move the finance-backed export providers from startup singletons to per-job construction. The engine now holds a ProviderFactory and the raw finance client instead of a ProviderRegistry; execute builds a fresh MemoizedFinanceClient plus a fresh provider set per job, so tags, budget_periods, default_settings, and the expenses tag map all share one GetAllUserData call. The expenses provider's buildTagMap derives its tag map from GetAllUserData().GetTags() instead of ListTags. Collection stays serial (fan-out is a later slice). Registration/ZIP order is unchanged (profile, expenses, tags, budget_periods, default_settings) and provider constructor/Collect signatures are unchanged. main.go wires the export providers through the factory; the deletion engine's separate registry is unaffected. The now-unused export ProviderRegistry is removed. Pre-dedup GetAllUserData=3 + ListTags=1 collapses to GetAllUserData=1 + ListTags=0 (BenchmarkExportCollection); byte-identical CSV goldens still pass. --no-verify: pre-commit backend lint trips on unrelated in-flight work in services/expense and services/gateway; this module lints clean (0 issues).
TestExport_DedupesFinanceCalls runs the real provider set through the engine with a finance spy embedding perf.CallCounter and asserts GetAllUserData is called at most once and ListTags zero times per export. Verified that bypassing the per-job MemoizedFinanceClient makes it fail (GetAllUserData=4). Rides the existing test-backend job; no new CI job. --no-verify: pre-commit backend lint trips on unrelated in-flight sibling work (services/expense, services/gateway); datarights builds/vets/lints/-race clean in isolation.
Adds BenchmarkExportExpenseRead comparing the OLD OFFSET export path
against the NEW keyset path at P in {1,10,50,100}, reporting the portable
structural signals (queries/export, counts/export) alongside a
same-machine wall-clock reference. Commits the baseline characterization
at perf/baseline/read.txt: OFFSET issues 2*P queries with P COUNTs and
rescans prior pages (O(P^2) rows), while keyset issues P queries with
zero COUNTs and no rescan (O(P)). The real scan-cost curve is confirmed
against real immudb by the integration test; the recording mock proves
query shape/count, not scan cost.
Adds a build-tagged (integration) test that runs GetExpensesByUserAfter against a real codenotary/immudb:1.11.0 container to confirm the unit query-shape assertions on the pinned production version: multi-column ORDER BY created_at ASC, id ASC and the expanded-OR (created_at, id) keyset predicate both work, ordering is correct across shared-created_at boundaries (no duplicates, no skips), and no query uses OFFSET. Skips cleanly when immudb is unreachable; excluded from the default test-backend job (no new CI job).
After the export dedup, no provider test exercises the finance mock's ListTags canned-data fields. Remove listTagsResp/listTagsErr and reduce the ListTags mock method to a no-op; it stays only to satisfy the FinanceServiceClient interface.
The StreamAllUserExpenses handler returned bare context.Canceled / context.DeadlineExceeded, which gRPC maps to codes.Unknown. Normalize them with status.FromContextError so clients see codes.Canceled / codes.DeadlineExceeded. Observability-only; stream-send failures still surface directly. Adds a handler test for the cancel path.
- add BenchmarkEngineCollectionFanout over five providers with 1x..5x simulated latency, driving engine.execute end to end - record the pre-fan-out serial baseline (~155ms ~= sum) in perf/baseline/export.txt as the reference for the fan-out change
- replace the serial provider loop in execute with errgroup.WithContext writing into a pre-sized, index-addressed csvFiles slice, cutting collection latency from sum(providers) to max(providers) - wrap each provider error with its name inside the goroutine and, after Wait, recheck ctx to keep the timeout vs collect-failure distinction - promote golang.org/x/sync to a direct require; record the ~155ms->~51ms (sum->max) result in perf/baseline/export.txt
- assert ZIP order stays registration order when providers finish in reverse, and that all providers run concurrently (max, not sum) - assert a named provider error survives errgroup first-error capture without leaking the cause, and a deadline maps to Export timed out - assert failJob persists via a fresh background context after the job context expires; unit-test humanCollectMessage parsing and fallback
- reword the provider-name wrap comment in execute for readability
Replace the paged, buffer-all GetAllUserExpenses read in ExpensesProvider with a Recv() loop over the new StreamAllUserExpenses server stream. Rows are formatted with the existing formatRow (reusing the #5 shared tag map) and emitted to a sink as they arrive via a new streamExpenses primitive that holds at most one row in flight, so a caller writing each row onward stays at O(pageSize) instead of buffering the whole raw-proto history. Collect keeps its DataProvider signature (works with the serial and the errgroup fan-out engine) by using an append sink. The stream is chronological (created_at ASC, id ASC), so CSV output is byte-identical to the pre-cutover export for the same seeded data. The unary RPC and OFFSET repo method remain until #9. Test doubles updated to serve the stream: the providers mock and the engine-package stubExpenseClient (keeps the byte-identical + dedup tests exercising the real provider).
Add a bounded-allocation growth-ratio regression (US-RD-03): with a discarding CSV sink the streamed consumer retains nothing, so peak retained heap stays flat (0 bytes) as the row count grows 50x (1,000 -> 50,000), while the pre-cutover buffer-all shape retains O(N) (~2.1MB) and blows past the bound -- reverting the cutover fails the test. Also add a mid-stream cancellation test (stops promptly, does not drain) and an allocation benchmark. Commit the baseline under perf/baseline/. -race clean; no new CI job.
…mory-bound scope Review should-fixes for the stream-consumer cutover: - streamExpenses derives a cancellable context (defer cancel) so the gRPC client stream is torn down on every return path, including an early emit-error return. The errgroup path already cancels gctx, but a direct / context.Background() caller would otherwise leak the stream. Matches #7's server-side teardown and the streaming-correctness single-owner contract. - Clarify the memory test comment and baseline note: the O(pageSize) bound is a property of the streamExpenses primitive plus a non-buffering sink, NOT of production Collect, which keeps the [][]string contract (and the engine buffers before BuildZIP) so end-to-end export memory stays O(total) until the follow-up engine-level cutover.
- delete unary GetAllUserExpenses service method and gRPC handler (superseded by StreamAllUserExpenses; sole consumer cut over in #8) - delete OFFSET-based GetAllExpensesByUser repo method with its per-page COUNT(*) from the repository interface and immudb impl - drop the OFFSET benchmark arm and vestigial OFFSET simulation in the recording test client; keyset-path coverage retained
- remove GetAllUserExpenses RPC and GetAllUserExpensesRequest from expense.proto and regenerate expensepb (protoc-gen-go v1.36.11, protoc-gen-go-grpc v1.6.1) - prune the now-dead GetAllUserExpenses stubs from the three datarights expense-client test mocks - BREAKING CHANGE: the unary GetAllUserExpenses RPC is removed; safe because the datarights export consumer cut over to StreamAllUserExpenses in #8
- update the datarights-export sequence diagram: the expenses read is now StreamAllUserExpenses (server-streaming), not the deleted unary GetAllUserExpenses (paginated) RPC - reflect the consumer's stream.Recv() loop with incremental writes instead of a buffered paginated response, keeping the diagram internally consistent
- describe concurrent errgroup collection in the data-export flow intro - wrap the provider reads in a par block with a fan-in barrier note - leave the already-fixed StreamAllUserExpenses line unchanged
- replace removed export ProviderRegistry with the per-job ProviderFactory - note the per-job MemoizedFinanceClient dedupe and errgroup collection - mark the export flow intro as concurrent, not serial
- the latency epic added a require+replace on services/perf to finance and datarights go.mod, so GOWORK=off go mod download reads ../perf/go.mod during the image build; neither Dockerfile copied it, failing the e2e job's docker compose --build with "reading ../perf/go.mod: no such file" - copy perf/go.mod (module-cache stage) and perf/ (source stage) in both Dockerfiles, mirroring the existing shared-module pattern - perf is stdlib-only with no go.sum, matching the healthcheck copy form
- drop 'see spec §09/§04' citations, priority codes (P2a/P4a/P4b/P3b),
and user story codes (US-EXP-01/US-RD-03) — all reference a desktop
spec file that doesn't travel with the code
- replace ticket-relative language ('this ticket', 'a later slice',
'#7 teardown', 'pre-cutover') with self-contained descriptions
- replace the "collect <name>: <cause>" string round-trip with a typed *collectError; recover the provider name via errors.As in the post-Wait switch instead of re-parsing the error string - Unwrap to the cause so the existing context.DeadlineExceeded timeout classification stays unchanged - drop humanCollectMessage and TestHumanCollectMessage; the end-to-end fan-out tests already cover the named-error and timeout paths
- clarify GetAllUserData ignores the request and opts of calls after the first, so every caller of an instance must pass the same request - record that this is safe today because each instance serves one single-user export job
- Replace direct *service.ServiceError type assertions in the stream handler and mapServiceError with errors.As, so a wrapped ServiceError maps to its gRPC status instead of falling through to codes.Unknown
- Buffer the rows channel to pageSize so the producer prefetches page N+1 while the consumer sends page N, keeping retained memory O(pageSize) - Add a deterministic test proving the producer blocks after ~one page of look-ahead when the consumer stops, not draining the full history - Point produceExpensePages at the datarights growth-ratio test that verifies the end-to-end retained-heap bound
- Replace idx_expenses_user_created (user_id, created_at) with idx_expenses_user_created_id (user_id, created_at, id) so the index matches the full ORDER BY created_at ASC, id ASC tuple and rows tied on created_at seek rather than scan+sort
- Convert the legacy b.N loop to for b.Loop() for parity with the P2 benches and drop the manual ResetTimer - Bracket per-iteration client setup with StopTimer/StartTimer so only the keyset walk is timed - Correct the rows-scanned comment: immudb 1.11.0 has no EXPLAIN, so the O(P) scan shape is analytical, not verified by the integration test
- Assert the full-export COUNT(*) count is Zero instead of <= 1; the keyset path issues exactly zero COUNT queries, so a stray per-page COUNT regression now fails instead of slipping past <= 1
- Fix receiver-method spacing so the integration-tagged file is gofmt-clean; a CI gofmt gate covering the integration build tag would otherwise fail (go test ./... alone does not compile it)
- Document at the StreamAllUserExpenses proto site that replacing the unary GetAllUserExpenses RPC (renamed, request swapped, field number 2 reused) is an intentional atomic break with no cross-version wire path
- Remove the wrong-direction mock wall-clock block (keyset appeared slower than OFFSET, a recording-mock artifact); keep the structural query-shape curve as the real evidence - Reframe rows-scanned as analytical: immudb 1.11.0 exposes no EXPLAIN; cite the covering index; the integration test confirms ordering and no-OFFSET, not scan cost - Point to the datarights growth-ratio test for retained-heap proof and explain why no misleading mock alloc pprof is committed
- add TestGetSpendingTrends_FanOutCancelsSiblings so the spec-09 first-error cancellation row is covered; the fake now checks its context on entry and fails fast, so a gctx->parent-ctx swap in the production goroutine is caught (previously only the timeout path was) - tighten TestGetAllUserData_FanOutRunsConcurrently from maxConcurrent>1 to ==3 so a regression lowering the fan-out below three reads fails
- replace the legacy for i := 0; i < b.N loop and manual b.ResetTimer() with for b.Loop() in BenchmarkGetAllUserData, BenchmarkGetSpendingTrends, and BenchmarkGetHistoricalComparison - matches the go 1.26 idiom already used by the datarights sibling benches
- fanout.go: list GetAllUserData as a current caller (not "later") and reframe the rationale to lead with the uniform shared-cap decision, noting the pgxpool sizes the cap for GetAllUserData's pg reads while the dashboard paths adopt it for their wide gRPC fan-out - alluserdata.go: drop the pgxpool-starvation prose that implied a wide fan-out here; state this fixed 3-read fan-out never reaches the cap
- re-capture wall-clock at count=10 (perf-methodology §3) on the same machine; add the fan-out side alongside the serial baseline for a complete A/B reference - document the allocs/op rise (errgroup + goroutine closures + wrapped error funcs: months=12 33->73, historical 12->29) as the reference-only cost of the latency win, not a gated metric - note the serial baseline is reproducible at commit 6036abb
- add stub authpb client to exercise the real GRPCTokenValidator, which the access-package tests never run (they use fakeValidator) - table-test field mapping to TokenValidationResult incl. the assumed-session AssumedBy case - assert a non-deadline client error is wrapped and stays 401 (not 503) end-to-end
- replace the legacy for i := 0; i < b.N loop with for b.Loop() on go 1.26, matching the migrated datarights/finance benches - drop the now-redundant b.ResetTimer() (b.Loop() excludes pre-loop setup); keep b.ReportAllocs() and the rec.Code check
- reframe defaultValidateTimeout as a hung-dependency backstop bounding the catastrophic-hang tail; state p99/tail control is out of scope for P1 and supersede the stale 2-5s band comment (GW-3) - warn at load when GATEWAY_VALIDATE_TIMEOUT exceeds a 30s recommended ceiling; no hard clamp, value preserved, so a 1h typo is visible (GW-4) - value/logic of the default 3s bound unchanged; add tests for the warn above/at the ceiling
- document the 3s bound as a hang backstop, not p99 tail control; p99 tightening toward auth P99 is future work, out of scope for P1 (GW-3) - record that the 503 timeout path is diagnosable via the distinct dependency=auth warn, and a DeadlineExceeded metric is a deliberate out-of-scope deferral per spec 04/12 (GW-2)
- rename max param to maxAllocs to avoid shadowing the builtin - trim allocRunsPerAssertion comment to the durable invariant - note AssertMaxAllocs is a shipped-but-unconsumed measurement helper
- add reader goroutine hitting Count/Total during concurrent Record - back the all-methods-safe claim under the race detector - make the CallCounter doc self-contained, dropping bare P2/P4 labels
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.
Summary
Delivers the LatencyEfficiency epic (tickets #1–#9): a set of per-request latency and efficiency improvements across the Go services, each shipped baseline-first and defended by a deterministic regression assertion that fails if the change is reverted.
The guiding rule (spec §04) is prove-don't-assume: gofin runs on a single VPS with ~5 concurrent users and 3-day Prometheus retention, so neither production traffic nor wall-clock time is a durable signal. The metrics committed and gated on are the ones that reproduce on any machine: upstream call counts, SQL query shape, and allocation bounds/ratios. Wall-clock numbers are kept only as same-machine references, never CI thresholds.
Headline results: gateway
ValidateTokengains a bounded timeout (no more unbounded tail latency); the GDPR export stops re-fetching finance data (4→1 finance call per export) and collects providers concurrently (sum→max); the expense export read drops fromO(P²)toO(P)via keyset pagination + server streaming; and the finance dashboard fans out its reads.What changed
Measurement foundation (
services/perf, P0)metrics/access/healthcheck, registered inservices/go.work. ProvidesCallCounter(concurrency-safe spy counter embedded in test doubles) andAssertMaxAllocs(arch-stable allocation bound with headroom), plus a documented bench/pprof/benchstat workflow. Consumed by the call-count and alloc-bound assertions in finance, datarights, and expense, so the backbone is load-bearing, not decorative.Gateway
ValidateTokenbounded timeout (P1)GRPCTokenValidatornow bounds everyValidateTokenRPC with a configurablecontext.WithTimeout(envGATEWAY_VALIDATE_TIMEOUT, default3s; wired indocker-compose.yml). A hung auth service returns fast instead of blocking a gateway worker indefinitely.context.DeadlineExceededor a gRPCDeadlineExceededstatus) maps to 503 Service Unavailable, distinct from the unchanged 401 for a genuine token rejection, with a warn log that names theauthdependency. Every other error still returns 401.Finance dashboard +
GetAllUserDataread fan-out (P4)GetSpendingTrends/GetHistoricalComparisonandGetAllUserDatanow collect their independent reads via a boundederrgroupinstead of a serial loop: collection latency becomesmax(reads)rather thansum(reads).Datarights export: finance-call dedup + errgroup fan-out (P2)
ProviderFactory, and the finance-backed providers share one per-jobMemoizedFinanceClientsoGetAllUserDatais fetched at most once per export andListTagsdrops to zero (finance calls 4→1). A fresh instance per job prevents cross-user data leakage (a startup singleton would leak the first user's data to all).errgroup; each goroutine writes only its own pre-assigned slice index, so ZIP order stays deterministic (registration order) regardless of completion order. First error wins and cancels siblings; a post-Waitcontext recheck keeps "Export timed out" distinct from "Failed to collect X data".Expense export read: keyset pagination + server streaming (P3)
GetExpensesByUserAfterseeks past a(created_at, id)cursor with an expanded-OR predicate (immudb 1.11.0 rejects row-value tuple syntax) andORDER BY created_at ASC, id ASC, fetchingpageSize+1to derivehasMorefrom the overflow row. NoOFFSET, no per-pageCOUNT(*): queries per export drop from2PtoP, rows scanned fromO(P²)toO(P).StreamAllUserExpensespages internally with the keyset cursor and streams rows; the datarights export consumer is cut over to it and consumes incrementally (O(pageSize)server memory). Validated against a livecodenotary/immudb:1.11.0container.GetAllUserExpensesRPC and the OFFSET-basedGetAllExpensesByUserrepo method are removed in a final bisectable cleanup.Documentation
docs/architecture.mdanddocs/data-export.mdupdated to reflect server-streaming expenses and the concurrent (errgroup) export provider fan-out with the per-jobProviderFactory+MemoizedFinanceClientmodel.Measurement discipline
Every optimization committed a baseline under
services/<svc>/perf/baseline/*.txtcapturing the environment-independent signal as primary content (call counts, query shape, retained-heap ratios), with wall-clock/absolute allocs clearly labeled as same-machine references. The durable regression gates ride the existingtest-backendCI job as ordinaryTest*functions (no new workflow, no wall-clock SLA gate): call-count assertions, query-shape assertions (noOFFSET,COUNT(*)≤1, one data query per page, linear query-count growth), and allocation growth-ratio bounds.-raceand the-tags integrationimmudb test are local gates per spec §04.Known follow-up (out of scope)
The P3 latency/algorithmic wins are fully realized in production (keyset
O(P²)→O(P)activated by the consumer cutover). One residual memory item remains: the export engine still buffers each provider's rows beforeBuildZIP, so end-to-end export peak memory staysO(total)until a futureDataProviderrow-sink + engine-loop +BuildZIPchange. This is a memory concern outside the epic's stated latency goal and is tracked as a documented follow-up.Test / validation
services/go.workworkspace green at HEAD:go build ./...,go vet ./...,go test ./...per module (incl.services/perf),go test -race ./...on the touched modules, andgolangci-lint run(v2.12.1) →0 issues.-tags integrationbuilds and the immudb keyset integration test compiles.