Repository navigation
feat(dashboard): decouple dataengine provider and parallelize multi-plot queries - #1407
Merged
AntonTyb merged 5 commits intoOct 27, 2026
Conversation
…lot queries Generalize `internal/dashboard/dataengine` engine construction, lifecycle management, and multi-plot execution so analytics backends beyond ClickHouse can plug into `DataEngine` and `Provider` without duplicating readiness, singleflight caching, or graceful teardown logic. - Introduce internal `rowScanner` and `queryBackend` interfaces in `internal/dashboard/dataengine` with `clickHouseBackend` and `DataEngine.Close()`. - Execute multi-plot `FetchChartData` queries concurrently via a bounded `errgroup` (`maxConcurrentPlots = 8`) while running single-plot charts inline and preserving deterministic `chart.GetPlots()` output slice order. - Add backend-agnostic `EngineBuilder`, `EngineResolver`, `NewEngineProvider`, `NewDynamicEngineProvider`, and `Provider.Close()`, delegating `NewProvider` and `NewDynamicProvider` through them and wiring `defer de.Close()` in `cmd/dashboard_service`. TAG=agy CONV=0fafd02b-f3b1-4563-a73d-49ec0845afbd Change-Id: I22e0e82bdeef95d5dcdef8a3b06dd88e2ec4f43b
There was a problem hiding this comment.
Code Review
This pull request refactors the dashboard data engine to abstract the database backend using a new queryBackend interface and generalizes the provider to support backend-agnostic engine building. It also introduces concurrent plot data fetching using errgroup, adds resource cleanup with a Close method, and expands test coverage. The review feedback correctly identifies a potential nil pointer dereference in fetchPlotData when a chart contains a nil plot element, recommending a defensive nil check to prevent runtime panics.
janzak-dev
reviewed
Oct 26, 2026
This was referenced Oct 26, 2026
…at/dataengine-query-backend-abstraction Change-Id: I2d024f1bba0599cb3f6122d869b46b09e94d9fbf
…ovider - Clamp `req.GetChartWidthPixels()` to `maxChartWidthPixels` (`4000`) in `FetchChartData`. - Remove duplicate `conn ClickHouseConn` field from `DataEngine` and unused `QueryRow` from `ClickHouseConn` / `mockClickHouseConn`. - Deduplicate dynamic engine rebuilds via `singleflight` (`DoChan`) with `ctx.Done()` caller cancellation abort and per-fingerprint `retryInterval` rate-limiting so `/readyz` probes never pile up behind slow dials. - Assert `logging.Warn` output during dynamic resolution and rebuild failures in `provider_test.go`. TAG=agy CONV=0fafd02b-f3b1-4563-a73d-49ec0845afbd Change-Id: I1a243b1b0446ff373cabe24b0453bc29e1780645
janzak-dev
previously approved these changes
Oct 27, 2026
Delegate `Provider.Ping` and `Provider.pingDynamic` tail checks to `pingActive(ctx, errProviderClosed)` so a concurrent `Provider.Close()` (`p.engine.Swap(nil)`) returns `errProviderClosed` cleanly instead of relying on concrete nil-receiver dispatch. Also short-circuit `beginAttempt` and `beginRebuildAttempt` when `p.closed` is true so post-`Close` calls within `retryInterval` never return `(nil, nil)` or redial a closed provider. Comment link: #1407 (comment) Change-Id: I177b1b7ab286554f619ed3c2b5d816db4ff7e522
There was a problem hiding this comment.
🤖 Incremental PR Approval
PR author is a code owner and changes since last approval (17 lines, excluding 95 test file lines) are within the 50-line grace delta.
Preserving prior approval by @janzak-dev on commit 0d4029c.
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
Introduces the package-internal
queryBackend/rowScannerexecution seam onDataEnginealongside exportedEngineBuilder/EngineResolverconstructors onProviderandDataEngine.Close()teardown, preparinginternal/dashboard/dataengineforNewBigQueryin follow-up PR #1408 without exposing internal scanner interfaces. Also parallelizes multi-plot chart queries with a boundederrgroupwhile preserving deterministic output ordering.Motivation & Context
DataEngineandProviderpreviously coupled connection dialing, dynamic configuration reload, and query execution directly toClickHouseConn. Decoupling query execution behind an internal backend contract, generalizingProviderontoEngineBuilder/EngineResolver, and parallelizing multi-plotFetchChartDatacalls preparesdashboard_servicefor higher-latency cloud analytics backends such as BigQuery (which incurs ~200–800ms per query) without duplicating singleflight caching, readiness probes, or graceful connection teardown.Key Changes
rowScannerandqueryBackendinterfaces withclickHouseBackendandDataEngine.Close()ininternal/dashboard/dataengine/querier.goandinternal/dashboard/dataengine/engine.go, plusmaxChartWidthPixelsrequest clamping.FetchChartDataqueries concurrently via a boundederrgroup(maxConcurrentPlots = 8) while executing single-plot charts inline and preservingchart.GetPlots()output slice order.EngineBuilder,EngineResolver,NewEngineProvider,NewDynamicEngineProvider, andProvider.Close()ininternal/dashboard/dataengine/provider.go, deduplicating dynamic rebuilds viasingleflightwithctx.Done()caller abort andretryIntervalrate-limiting while delegatingNewProviderandNewDynamicProviderfor full backward compatibility.defer de.Close()duringcmd/dashboard_serviceshutdown and add unit tests inquerier_test.go,engine_test.go, andprovider_test.go.Assumptions
maxConcurrentPlots = 8balances multi-plot dashboard latency against backend connection/slot concurrency limits — chosen because BigQuery interactive queries share a per-project concurrent query quota and ClickHouse connection pools are finite — if charts routinely contain dozens of plots under low concurrency, increasing or parameterizing this cap may improve tail latency without breaking API contracts.Verification & Testing
bazel test --runs_per_test=10 --@io_bazel_rules_go//go/config:race //internal/dashboard/dataengine/... //cmd/dashboard_service/...,bazel coverage --combined_report=lcov //internal/dashboard/dataengine/...,./.githooks/pre-commitRelated Issues / Bugs
N/A
TAG=agy
CONV=0fafd02b-f3b1-4563-a73d-49ec0845afbd