tests: faster dev loop + hpack dep dedup; misc hasql fixes#411
Conversation
… misc fixes Squashed from 19 commits on telemetry-derive-row-instances. Backup branch: telemetry-derive-row-instances-backup.
Move the library dependency list into hpack-includes/lib-deps.yaml and reference it from both 'library' and 'tests.test-dev' via !include. Eliminates the manual mirror that drifted (vector-split was missing from test-dev, breaking live-test-dev after rebase). hpack 0.38 does not splice list-position !include, so test-only deps (hspec, uuid-quasi, ki, pg-transact, aeson-pretty, process) also live in the shared file; the library declares them unused, silenced by the existing -Wno-error=unused-packages. Document make live-test-dev + the include-file pattern in CLAUDE.md (local-only; CLAUDE.md is gitignored in this repo).
ReviewThe core ideas here — Bug:
|
| Line | Annotation | SQL |
|---|---|---|
| 656 | [(ProjectId, Text, Int)] |
COUNT(*)::bigint |
| 677 | [(ProjectId, Text, Int)] |
COUNT(*)::bigint |
| 757 | [(ProjectId, Text, Text, Int, Int)] |
::bigint AS reported, ::bigint AS ingested |
These will decode fine on 64-bit Linux (where Int is 64 bits) but the decoder contract is technically violated — hasql's int8 decoder returns Int64, not Int. Worth fixing in this PR since you're already in this file for the same reason.
live-test-dev missing from .PHONY
The new target isn't in the .PHONY declaration (line 244). If a file named live-test-dev ever exists in the repo root, make will silently skip running the recipe.
-.PHONY: ... tmux-pin-here tmux-unpin ...
+.PHONY: ... live-test-dev tmux-pin-here tmux-unpin ...Minor: TEST_MATCH is unquoted in the match flag
--test ':main $(if $(TEST_MATCH),--match=$(TEST_MATCH))'If TEST_MATCH contains spaces or glob characters the shell will mis-parse it. --match="$(TEST_MATCH)" (inner double-quotes) would be safer, since the whole string is already inside single quotes for ghcid:
--test ':main $(if $(TEST_MATCH),--match="$(TEST_MATCH)")'Observation: test-only deps in the library
The comment in lib-deps.yaml is clear and the -Wno-error=unused-packages suppression is already repo-wide, so this is understood to be a hpack 0.38 workaround. One alternative for the record: ki is already in the library as ki-effectful (which re-exports the structured concurrency primitives), so the bare ki entry might be droppable from the extras list depending on what test/integration/ actually imports.
2364483 to
cf8db3f
Compare
ReviewOverall this is a solid DX/infra PR. A few concrete things worth addressing:
|
… Makefile fixes - hpack-includes/lib-deps.yaml: remove the second base16-bytestring entry (was a carry-over from the old package.yaml). - BackgroundJobs.hs: same Int -> Int64 fix as 66a565a applied to the three remaining COUNT(*)::bigint sites (issueCounts, alertCounts, usage audit rows). - Makefile: add live-test-dev to .PHONY; quote TEST_MATCH in the --match arg so values with spaces / glob chars don't get shell-mangled.
|
See review below |
- BackgroundJobs.stuck: headDef 0 instead of fromMaybe 0 . listToMaybe (from Data.List.Extra, already in deps). - test-dev ghc-options: drop -threaded (already in global ghc-options; hpack was concatenating to a duplicate flag). - Makefile @build-pane: -w (window-scoped) instead of -g (global). Two project windows pinning concurrently no longer clobber each other. (Reviewer also noted hspec/uuid-quasi/ki/pg-transact/aeson-pretty/process leak into the library's build graph via lib-deps.yaml; that's the hpack-0.38 list-splice limit and can be revisited if/when hpack ships proper support — left as-is for now.)
|
Code Review - PR 411 Overview: Adds a test-dev GHCi target (src/ + test/integration/ as one unit), consolidates lib deps into hpack-includes/lib-deps.yaml, fixes Int->Int64 for COUNT()::bigint, updates doctest pin, adds haddock to DeriveUtils. Bug - proto-lens-protoc missing from package.yaml test-dev build-tools: package.yaml lists only hspec-discover:hspec-discover, but the generated cabal has both hspec-discover and proto-lens-protoc. Since test-dev compiles all Proto modules via generated-other-modules, the next hpack run would drop proto-lens-protoc and break the build. Add it explicitly or confirm it comes from a global build-tools stanza. Generated Cabal: Duplicate Flags: test-dev ghc-options contain both -fwrite-ide-info and -fno-write-ide-info, and -threaded twice. Global options prepend before stanza overrides. Last occurrence wins so it is functional, but noisy. Test-Only Deps Leaking into Library: lib-deps.yaml adds hspec/uuid-quasi/ki/pg-transact/aeson-pretty/process to the shared file, silenced by -Wno-error=unused-packages. Accepted given the hpack 0.38 limitation, but it permanently degrades -Wunused-packages signal on the library. Consider a separate hpack-includes/test-dev-extras.yaml with two !include lines in the tests: test-dev: dependencies block — the splice limitation may only affect the library: stanza. Makefile - tmux Option Namespace: tmux set-option -g @build-pane is machine-global across all sessions. Namespacing as @monoscope-build-pane avoids clobbering other projects. Positive: lib-deps.yaml dep consolidation is clean and fixes the vector-split drift; tmux_run pane-id tracking is more robust than grepping command names; Int->Int64 for COUNT bigint is correct; doctest pin and haddock additions are good. |
Code ReviewOverviewClean dev-loop improvement: the combined Correctness / Bugs
Biggest Concern: Test-only Deps Leaked into Library
# === extras shared with test-dev ===
- hspec
- uuid-quasi
- ki
- pg-transact
- aeson-pretty
- processThese are now declared as explicit library
Suggestion: Keep the shared file as just the pure lib deps and list the 6 test-only extras inline in the # package.yaml, test-dev.dependencies:
dependencies: !include hpack-includes/lib-deps.yaml
# test-only extras:
- hspec
- uuid-quasi
- ki
- pg-transact
- aeson-pretty
- processThis is exactly what the existing MakefileTmux pane pinning — Using
DeriveUtils.hsThe two doc comments on Summary
The core idea is solid. The main thing worth addressing before merge is keeping the 6 test-only extras out of the library's dep graph — they are importable in prod code with no warning once they are in |
Code ReviewOverall this is a clean, well-motivated PR. The Factual correction to the existing review comment The previous review flags four issues — all four are already addressed in this diff:
Genuine observations BackgroundJobs.hs — headDef
lib-deps.yaml — test-only deps leaking into library stanza The workaround (hspec, uuid-quasi, ki, pg-transact, aeson-pretty, process appearing in the library's declared deps) is documented and silenced by package.yaml test-dev — proto-lens-protoc absent from build-tools The generated Makefile — log file gitignored?
Summary The Int→Int64 fixes are correct and complete across all affected queries, dep deduplication via |
After merging master (which added the test-dev stanza in #411), the auto-generated other-modules list was missing Pkg.Metrics — the stanza was hpack-generated before this branch's new module existed.
…ncy, redactJSON early-exit (#412) * ingestion: OTel histograms for decode + write phase Initialises a global MeterProvider in Start.startApp (nested inside the existing TracerProvider bracket so shutdown order is meter→tracer, both flushing pending exports via shutdown*Provider). Picks up the same OTEL_EXPORTER_OTLP_ENDPOINT / headers as traces by default, with the usual per-signal overrides. Adds Pkg.Metrics with two top-level NOINLINE instruments (monoscope.ingest.decode.duration, monoscope.ingest.write.duration, ms) plus a `timed` helper that brackets the action so latency is recorded on exception too — useful for the dual-write where a timeout is still latency we want in the distribution. ProcessMessage.processMessages now records the decode phase manually and wraps insertAndHandOff in `timed`. * Auto-format code with fourmolu * ingestion: per-partition Kafka group concurrency Group records by (topic, partition) instead of just topic and process groups via pooledForConcurrentlyN with a new kafkaGroupConcurrency knob (default 4). Each tpsFor still yields exactly one TP, so a failed group only stalls its own partition's commits. Bound stays well under the hasql pool (shared with web): the failure mode to avoid is AcquisitionTimeout → Hasql.isTransientException → no commit → redelivery storm. Single committer (the poll thread) — no concurrent commitPartitionsOffsets. * ingestion: symmetric Metrics.timed for decode + write, redactJSON elem idiom Wraps the decode block in Metrics.timed (replacing manual getMonotonicTime + recordMs) so both phases use the same bracket-on-exception path. Drops the GHC.Clock import. redactJSON: switches `any T.null ps` to `"" \`elem\` ps` — equivalent but reads as "have we consumed all path segments?" which matches intent. Removes TODO.md — belongs in PR/issue tracker, not the repo. * hpack: pick up Pkg.Metrics in test-dev other-modules After merging master (which added the test-dev stanza in #411), the auto-generated other-modules list was missing Pkg.Metrics — the stanza was hpack-generated before this branch's new module existed. * queue: use HashMap consistently for byTP + tpsFor, drop Data.Map import The rest of Pkg/Queue.hs uses HM throughout; (Text, Int32) and Int32 keys are both Hashable, so HashMap is a drop-in. tpsFor's group is always single-partition, so the map has size 1 — ordering doesn't matter. * queue: revert HashMap swap, KP.PartitionId has no Hashable instance The previous 79bd1bc assumed crPartition was Int32 (Hashable), but it's KP.PartitionId (Ord-only). Data.Map.Strict was load-bearing, not an inconsistency. Comment added to prevent the next reviewer from trying the same swap. --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Summary
make live-test-dev: lib + integration tests as one GHCi target via a newtest-devtest-suite, with--matchfiltering. Compilessrc/+test/integration/together so iteration on test code skips the library re-link step.hpack-includes/lib-deps.yaml: single source of truth for library dependencies, referenced from bothlibraryandtests.test-devvia!include. Eliminates the manual dep mirror (which had already drifted —vector-splitwas missing fromtest-dev, breaking the build after rebase). hpack 0.38 doesn't splice list-position!includes, so a handful of test-only deps (hspec, uuid-quasi, ki, pg-transact, aeson-pretty, process) also live in the shared file; the library declares them unused, silenced by the existing repo-wide-Wno-error=unused-packages.Int64inBackgroundJobs.hs; doctest pin onlength otelColumns; minorDeriveUtilsadjustments.Test plan
hpackregenerates cleanlycabal repl monoscope(ghcid): All good, 109 modulescabal build test-dev: exit 0, 157 modules compiled and linkedmake live-test-devon a real spec (e.g.TEST_MATCH=/MonitoringSpec/ make live-test-dev)