fix(server): install telemetry as the FIRST step of boot, not the last - #419
Merged
Conversation
Telemetry used to install inside serve(), which build() calls only after doing all the real boot work (LaneStore, embedder, DuckLake attach, the writer lease, #563's lake adoption). Every log line from that entire sequence was silently dropped — no subscriber existed yet to capture it. Found chasing exactly that: #563's new "writer boot: adopted existing lake content before serving" log never appeared on lab despite other, independent evidence (the periodic publish task's dirty-check correctly skipping its first tick) proving the adoption code was genuinely running. Confirmed via `git show <deployed-sha>` that the log statement really was in the shipped binary — this was a boot- ordering gap, not a bug in #563's fix itself. install_telemetry now takes just a version string (all it ever read off ServerConfig) and EscurelConfig::build calls it as literally its first statement, before anything else. The resulting guard threads through to serve(), which now accepts an already-installed guard instead of always installing its own — falling back to installing one itself (`None` → install) for a caller that reaches serve() without going through build(), preserving existing behaviour there. Updated the one such caller (escurel-test-support's process harness, which constructs ServerConfig directly) to pass None explicitly. No functional change to anything build() does — same steps, same order — only WHEN logging becomes possible relative to them. Full suite: escurel-index 224/224, escurel-server 444/444 (no regressions — including telemetry_filter.rs, whose EscurelProcess harness bypasses build() and exercises the new None-fallback path). clippy -D warnings and fmt --check both clean across the workspace. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Problem
Telemetry used to install inside
serve(), whichEscurelConfig::build()calls only after doing all the real boot work (LaneStore, embedder, DuckLake attach, the writer lease, #563's lake adoption). Every log line from that entire sequence was silently dropped — no subscriber existed yet to capture it.Found chasing exactly that: #563's new
"writer boot: adopted existing lake content before serving"log never appeared onlabdespite other, independent evidence (the periodic publish task's dirty-check correctly skipping its first tick) proving the adoption code was genuinely running. Confirmed viagit show <deployed-sha>that the log statement really was in the shipped binary — this was a boot-ordering gap, not a bug in #563's fix itself.Fix
install_telemetrynow takes just a version string (all it ever read offServerConfig) andEscurelConfig::buildcalls it as literally its first statement, before anything else. The resulting guard threads through toserve(), which now accepts an already-installed guard instead of always installing its own — falling back to installing one itself (None→ install) for a caller that reachesserve()without going throughbuild(), preserving existing behaviour there. Updated the one such caller (escurel-test-support's process harness, which constructsServerConfigdirectly) to passNoneexplicitly.No functional change to anything
build()does — same steps, same order — only when logging becomes possible relative to them.Testing
Full suite:
escurel-index224/224,escurel-server444/444 — no regressions, includingtelemetry_filter.rs(whoseEscurelProcessharness bypassesbuild()and now exercises the newNone-fallback path).cargo clippy --all-targets -- -D warningsandcargo fmt --checkboth clean across the workspace.Plan after merge: deploy to
laband confirm the #563 adoption log line (and everything else from early boot) finally shows up in a real pod's logs.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.