feat(query-engine): QueryDef registry + 5 pilot handlers - #344
Merged
Conversation
Every handler in query-engine.http.ts wires its own cost profile, span
context, error label and -- if the author remembered -- caching. Caching
being opt-in per call site meant silence read as "off": 11 of 61 handlers
called cachedDirect, the other 50 are uncached by omission rather than by
decision. Nothing in the type system asked.
QueryDef makes those choices data instead of boilerplate. The load-bearing
field is `cache`, which is required-but-nullable: `cache: undefined` is a
decision someone made, a missing `cache` is a compile error. That is the
mechanism by which the remaining ~50 handlers become reviewed decisions as
they migrate.
runQuery/runQueryFirst apply the declared policy in one place. Rows-vs-first
-row stays a call-site concern rather than a field on the def, because the
same compiled query legitimately supports both and compiledQueryFirst takes
the identical CompiledQuery -- encoding it would only let a caller disagree
with it.
Deliberately out of scope: row decoding. Handlers keep their own
hand-coercion and response mapping. Folding that into a declared rowSchema
is a real improvement but a riskier, separate change -- it turns
ClickHouse's UInt64-as-JSON-string into a hard failure where it is currently
a silent Number("123"), and every migrated query has to clear the DESCRIBE
sweep. Keeping them apart means a decode regression can never be tangled up
with a caching change.
Pilot covers 5 handlers spanning both shapes and both cache states:
errorsByType, errorsTimeseries, errorRateByService (rows, uncached),
errorsSummary (first-row, uncached), serviceOverview (rows, cached ttl=15
version=2). Cache settings are carried over verbatim, so this changes no
caching behaviour. 56 handlers keep their inline wiring; the two styles
coexist while migration proceeds.
Registry lives behind the ./registry subpath, not the root barrel, since
entries pull in ./runtime for cache-policy types and the root barrel stays
driver-free for web/cli.
Verified: query-engine + apps/api typecheck; SQL baseline byte-identical
(14 tests) proving the migrated queries compile to the same SQL;
apps/api warehouse + v2 route suites 254 pass.
Takes the registry from 5 to 22 of 61 handlers. query-engine.http.ts loses 327 lines; the remaining handlers keep their inline wiring and the two styles continue to coexist. Entries were generated from the handler source rather than written by hand, after the pilot showed that transcribing them from memory silently dropped builder options. The generator asserts, per handler, that the region it replaces contains exactly one CH.compile, no Effect.all, and no statements other than the query plumbing -- so nothing but boilerplate is deleted. Two type changes fell out of real handlers rather than speculation: * `settings` may now be a function of the payload. listLogs only wants LOGS_BODY_SEARCH_SETTINGS when the caller actually passed a search term, and a static field could not express that. Resolved to a spread so an undefined result omits the key instead of passing `settings: undefined`, which downstream would read as "clear the profile defaults". * serviceHealthSnapshot and serviceApdex called cachedDirect with no explicit policy, i.e. its default of CACHE_SNAP_S. That constant is not exported, but DirectRouteCachePolicyInput accepts a bare number and the value is 15, so `cache: 15` is byte-equivalent to passing nothing. Seven candidates were deliberately NOT migrated: * cloudflareInfraZoneTimeseries / cloudflareInfraWorkerTimeseries build on @maple/query-engine-integrations, which depends on query-engine -- putting them in this registry would invert that dependency. * getLog, fleetUtilizationTimeseries, podInfraTimeseries, nodeInfraTimeseries and workloadInfraTimeseries compute values in the handler body (a spec, a bucket width, a time range) that `compile` cannot see. Moving that logic in is worthwhile but is per-handler judgement, not a mechanical rewrite. Caching behaviour is unchanged everywhere: every policy is carried over verbatim, and `cache: undefined` records "was uncached", not "should be". Verified: query-engine + apps/api typecheck; SQL baseline byte-identical (14 tests), proving all 22 migrated handlers emit exactly the SQL they did before; apps/api routes + warehouse suites 310 pass.
Three of the seven handlers skipped last round were skipped for a dependency reason, not a difficulty one: the Cloudflare queries come from @maple/query-engine-integrations, which itself depends on @maple/query-engine, so declaring them in the core registry would invert that edge. The rest needed helpers owned by the API app. Adds apps/api/src/routes/queries.ts, which spreads the core registry and adds entries that need app-side dependencies. Handlers import `Queries` from there, so the split is invisible at the call site and an entry can move between halves without touching a handler. Extracts apps/api/src/routes/query-helpers.ts for logic both sides need. The pod/node/workload metric switches are the interesting case: `compile` needs the metric name while the handler needs the unit for its response, so the switch has to be shared. Duplicating it would let the two drift -- the exact failure this registry exists to prevent. `cache` may now be a function of payload and current time. spanDetail needs it: a finished trace is immutable and cacheable, one still receiving spans is not, so its TTL comes from the requested end time against now. `nowMs` is supplied by the runner from the Effect Clock rather than read in the def, and the clock is only read when a def actually asks for it. cloudflareInfraZoneTimeseries keeps its local `filters` binding: the response reports which filters the metric family could not honor, which is presentation, not query construction. Verified: apps/api typecheck; SQL baseline byte-identical (14 tests) across all 30 migrated handlers; apps/api routes + warehouse suites 310 pass.
serviceUsage, serviceDependenciesBundle and serviceDetailOverview.
serviceDependenciesBundle is the first payoff of the unification rather than
just a refactor of it: two of its three sub-queries are the SAME builders as the
standalone serviceDependenciesForService and serviceDbEdgesForService handlers,
so it now reuses their defs instead of re-deriving the compile inline. That is
one definition where there were two, which is the drift this registry exists to
stop.
Sub-queries of bundle endpoints keep their own ids -- an id is both the span
context and the cache-key prefix, so folding them under the bundle name would
merge unrelated cache entries.
Their payload types are the MINIMAL input each needs, not the bundle's full
payload. That is load-bearing rather than tidiness: runQuery keys the cache on
whatever payload it is handed, and serviceEnvironments was hand-cached on
{serviceName, startTime, endTime} rather than the whole request. Typing the def
narrowly reproduces that key byte-for-byte instead of silently moving every
entry.
serviceUsage folds its with/without-previous-window branch into compile, so the
id, profile and 60s TTL stay a single decision rather than being restated on
both arms.
Verified: apps/api typecheck; SQL baseline byte-identical (14 tests);
apps/api routes + warehouse suites 310 pass.
serviceDependencies, serviceDbEdges, serviceExternalEdges and serviceWorkloads. All four use *SQL(opts, params) builders, which return a CompiledQuery directly rather than going through CH.compile. serviceWorkloads keeps its empty-service-list guard in the handler: that path returns without issuing a query at all, which a def cannot express. The rewrite anchors on the handler's `const compiled` rather than on its response constructor. Anchoring on the response silently clobbered serviceWorkloads, whose early-return response appears BEFORE its query. Verified: apps/api typecheck; SQL baseline byte-identical; routes + warehouse suites 310 pass.
🍁 Maple PR previewNote Preview resources were removed when this pull request closed. Final commit |
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
1 participant
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.
First step of the query-layer unification. Establishes the declarative registry and proves it against 5 handlers; the other 56 are untouched.
Why
Every handler in
query-engine.http.ts(3275 lines, 61 handlers) wires its own cost profile, span context, error label and — if the author remembered — caching.Caching being opt-in per call site meant silence read as "off": 11 of 61 handlers call
cachedDirect; the other 50 are uncached by omission rather than by decision. Nothing in the type system ever asked.What
QueryDef(packages/query-engine/src/registry/query-def.ts) makes those choices data instead of boilerplate. The load-bearing field iscache, which is required-but-nullable:cache: undefinedis a decision someone made; a missingcacheis a compile error. That's the mechanism by which the remaining ~50 handlers turn into ~50 reviewed decisions as they migrate.runQuery/runQueryFirst(apps/api/src/routes/query-runner.ts) apply the declared policy in one place. A handler goes from ~25 lines to:Net: +8 / −95 in
query-engine.http.tsfor 5 handlers.Design notes for review
compiledQueryFirsttakes the identicalCompiledQuery; encoding it in the def would only let a caller disagree with it.Number(row.count),decodeFingerprintHash(...)). Folding that into a declaredrowSchemais a real improvement but a riskier, separate change — it turns ClickHouse's UInt64-as-JSON-string into a hard failure where it's currently a silentNumber("123"), and every migrated query has to clear the DESCRIBE sweep. Keeping them apart means a decode regression can never get tangled up with a caching change.serviceOverview'sttlSeconds: 15, version: 2and its reasoning.cache: undefinedon the other four means "was uncached before", not "should be uncached"../registrysubpath, not the root barrel, since entries pull in./runtimefor cache-policy types and the root barrel stays driver-free for web/cli.Two bugs caught while writing this
Worth noting since they're the kind the registry is meant to prevent, and I hit them writing the entries from memory rather than from the code:
errorsSummarytakes four builder opts (rootOnly,services,deploymentEnvs,fingerprintHashes) and usescompiledQueryFirst, notcompiledQuery.errorRateByServiceQuery()takes no arguments at all — it's scoped purely by org and time range.Both were caught by reviewing the generated diff before typechecking.
Testing
packages/query-engine+apps/apitypecheck cleanapps/apiwarehouse + v2 route suites: 254 pass, 126 skipped (ClickHouse e2e, needsch:up)Not run: the ClickHouse DESCRIBE sweep, which isn't needed here since no
rowSchemawas added and the emitted SQL is unchanged.Next
Migrate the remaining 56 in batches by query family, each with
cache: undefinedfirst so caching changes land separately from any decode work. Thenpipe-dispatch.ts(784 → ~150 lines) as a thin adapter, gated on the SQL baseline staying byte-identical across all 37 pipes.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.