Repository navigation
feat(api): make the background indexer observable and configurable from the embedded API - #1710
Conversation
The index-install write-lock section (apply_loaded_db novelty trim + dict_novelty rebuild + reseed_runtime_small_dicts) scales with accumulated novelty and blocks committers on the same lock, but had no timing of any kind. apply_index_v2 now runs under a debug span recording ledger_id and the incoming index_t, with a nested apply_index_install_lock span bracketing exactly the exclusive-lock window, so a commit-latency spike can be attributed to an index install.
incremental_max_commits, run_budget_bytes, leaf sizing, and data_dir were defaults-only through the builder: with_indexing_thresholds only ever carries an IndexerConfig::default() forward, and every escape hatch (build_with, with_indexing_mode, from_backend) silently drops the LedgerManager. The new setter installs an explicit IndexerConfig while preserving thresholds already set, mirroring with_indexing_thresholds' merge in the other direction.
IndexerHandle::status()/wait_for_idle() are exactly what an embedder's drain phase wants, but nothing built through FlureeBuilder could reach them without poking the raw indexing_mode field. Expose the handle directly.
… review Hardening pass from an internal review of the three preceding commits: - Pin `with_indexer_config` + `with_indexing_thresholds` composition in both orders with a unit test. One half of this merge was silently broken once before (e6d0044, guarded by the sibling threshold-propagation test); this guards the other half. - Assert the `indexer_handle()` None side on `build_memory()`, and switch `it_indexing_wait.rs` from poking the raw `indexing_mode` field to the new accessor — the accessor now has Some-side integration coverage, and the last out-of-impl reader of the raw field is gone. - Split the install-lock span's wait-vs-hold: the `apply_index_install_lock` span opens before `state.write().await`, so under contention its duration is acquisition wait plus hold; a debug event at guard acquisition marks the seam so the two segments can be separated in analysis. - Docs: state exactly which build paths `with_indexer_config` and `indexer_handle()` take effect on (build/build_s3*/encrypted) and which silently construct without an in-process indexer (build_memory, build_with, from_backend, with_indexing_mode); note that provider hooks on a supplied IndexerConfig are replaced by the API layer's wiring at build time; and correct `build_with`'s claim that it honors "indexing settings" — it hard-codes IndexingMode::Disabled.
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
Approving — but there are a few things I'd like you to look at before this merges, one of them a repo rule rather than an opinion.
I checked all three gaps against origin/main rather than taking the issue's word, and all three are real. The span gap in particular is understated if anything: the doc comment calls that section a "brief atomic swap of both state and binary_store", but it runs apply_loaded_db, sync_store_and_snapshot_ns and reseed_runtime_small_dicts, all novelty-scaled — and the contention claim is exact, not approximate: the install takes self.inner.state.write() and lock_for_write() at :319 takes Arc::clone(&self.inner.state).write_owned(), the same lock. The config gap checks out too — with_indexing_thresholds was defaulting indexer_config away, build_with (lib.rs:2116) and both build_memory arms (:2264, :2300) hard-code Disabled, and data_dir is never read or overridden anywhere in fluree-db-api, so it really was unreachable and really does flow through untouched now. The accessor gap is proven by your own test diff — it_indexing_wait.rs was reaching through the public field.
The shape of the change is good: purely additive, symmetric merge in both orders, field::Empty + record on the outer span is the right pattern, and both IndexerConfig and IndexerHandle were already re-exported (lib.rs:263) so the new surface is usable without adding a dependency. Deferring the duplicated novelty walk out of an instrumentation PR is right.
On performance, since that's the natural question for an instrumentation PR: the direct cost is nil. debug_span! is ~1-2ns disabled per docs/contributing/tracing-guide.md:15, field expressions aren't evaluated on a disabled callsite, and this fires once per install rather than per commit or per row — the opposite end of the spectrum from the query merge loops where CLAUDE.md records +5-15% for a never-executed branch. The refactor adds no scheduler yield and the guard still drops at the same statement. Two notes inline on where a cost could hide and on what the config knobs are actually worth.
Four items below. The span-doc sync (second comment) is the one I'd treat as blocking.
| // Splits the span into wait-vs-hold: the span opens before the | ||
| // write().await above, so under contention its duration is | ||
| // acquisition wait + hold; this event's timestamp marks the seam. | ||
| tracing::debug!("install lock acquired"); |
There was a problem hiding this comment.
The span opens before state.write().await, so its duration is acquisition wait plus hold — and hold is the number the issue is about. You've noted this and marked the seam with this event, but that mitigation needs per-trace arithmetic and doesn't aggregate, so the p99 dashboard that motivated the issue still can't answer the question.
That matters because the two halves point at opposite fixes. If the tail is acquisition wait, the answer is backpressure or install batching. If it's hold, the answer is killing the duplicated novelty walk. As written the span can't distinguish them, which is the one thing it was built to do.
Cheap to fix either way — acquire the guard before the instrumented block (it's Send, so moving it in works), or split into apply_index_install_wait / apply_index_install_lock for two independently aggregatable durations. The split also removes the need for this event entirely, which is worth something on its own: an event is a full subscriber dispatch, and this one fires inside the exclusive lock every committer blocks on. Against a ~350ms hold that's noise, and it only exists with debug logging on — but a fmt layer on a contended stdout is a mutex and possibly a blocking write inside the critical section you're trying to measure. Small, avoidable observer effect on exactly the wrong section.
There was a problem hiding this comment.
You're right on both halves, and the split is in (e42602cf5): the span is now two — index_install_wait instruments exactly the state.write() acquisition future, index_install_lock exactly the guarded section — so wait and hold aggregate independently and the p99 dashboard can finally point at the right fix. The debug!("install lock acquired") event is deleted.
On the observer-effect concern: we verified the mechanism against tracing-subscriber 0.3.23 (the lockfile version) rather than taking it on trust — fmt's on_event formats into a thread-local buffer and then does make_writer_for + write_all, which for the default stdout writer is stdout's lock plus a syscall that can block on a contended pipe, and for a Mutex<W> writer literally takes the mutex. So yes: that event was a formatted write + writer lock + potentially blocking I/O inside the exclusive state lock whenever a debug-level fmt subscriber was attached. Real, not hypothetical. Glad it's gone.
| ) -> Result<()> { | ||
| use tracing::Instrument as _; | ||
| let span = tracing::debug_span!( | ||
| "apply_index_v2", |
There was a problem hiding this comment.
Two new spans, and none of the six files CLAUDE.md requires are in the diff. I grepped for both names across .claude/, docs/ and the tests — zero hits anywhere.
Missing:
.claude/skills/trace-inspect/references/span-hierarchy.mdand.claude/skills/trace-overview/references/span-hierarchy.md— the tree there is detailed enough thatindex_buildand its children are itemized, so an install span is conspicuous by absence- both copies of
trace_common.py(EXPECTED_CHILDREN/EXPECTED_CROSS_THREAD/KNOWN_ZERO_DURATION) docs/operations/telemetry.mdfluree-db-api/tests/it_tracing_spans.rs— 1087 lines of exactly this kind ofhas_span+ level acceptance test
This is the item I'd hold the merge on, since it's a written repo rule and the trace-inspect skills go stale silently when it's skipped — there's no failing check to catch it later.
Separately, a naming thought while you're here: apply_index_v2 embeds a format version, and span names become dashboard and query dimensions. A v3 renames the operation and breaks saved queries against it. index_install would age better and reads better next to index_build.
One more: the gates list check/clippy/fmt but no cargo test, and this PR edits it_indexing_wait.rs and adds three unit tests. CI covers it — just below the bar the rest of the description sets.
There was a problem hiding this comment.
Complied with everything this repo can actually enforce — but verifying the rule first (rather than just following your list) turned up something you'll want to know about.
The six-file rule as quoted lives in db-r's CLAUDE.md (lines 199-206, verbatim including "BOTH copies"), not this repo's — fluree/db's CLAUDE.md has no tracing section at all, and .claude/skills/ has never existed in fluree/db's git history (checked git log --all and the GitHub contents API). Your zero-hits grep was true because four of the six files aren't here to hit. What fluree/db actually binds is docs/contributing/tracing-guide.md's "Checklist for New Instrumentation": the telemetry.md span tree + .claude/skills/*/references/span-hierarchy.md (a dangling reference, see below) + an emission test. The satisfiable set is therefore telemetry.md + it_tracing_spans.rs, and both are done — the new tree section with wait-vs-hold prose, and AC-10 (existence + level + both children's parentage + index_t recording + absence of the old names and the deleted event + zero-noise at INFO, on a real notify→install cycle).
The part worth your eyes: fluree/db carries dangling db-r inheritance — the tracing-guide checklist line and a comment in multi_query_tracing_integration.rs:8 point at skills that don't exist here, and telemetry.md's own span tree documents build_all_indexes / upload_indexes / index_gc etc., which appear in zero .rs files in this repo (the actual indexer spans are commit_chain_walk, dict_merge_and_remap, v3_rebuild, …). The db-r skill files themselves are real but already diverged between their two copies and describe db-r's hierarchy, so importing them here as-is would document the wrong repo. Two ways to square it — port-and-correct the skills into fluree/db as a dedicated PR, or fix the dangling references to match what exists — and that's your call rather than something to smuggle into three concurrent span PRs. Happy to take whichever direction you pick.
Also adopted here: your naming point — the spans are index_install / index_install_wait / index_install_lock now, no format version in a dashboard dimension. And fair hit on the gates list: the full cargo test -p fluree-db-api ran this round (AC-10 and the warn-flag tests included), alongside clippy/check/fmt.
| /// | ||
| /// Controls index-build parameters that are otherwise defaults-only | ||
| /// through the builder: `incremental_max_commits`, `run_budget_bytes`, | ||
| /// leaf sizing, and `data_dir`. Novelty thresholds already set via |
There was a problem hiding this comment.
Agreed this is the most valuable knob here, and it's worth being precise about why, because the config doc it inherits oversells it in one direction and undersells it in another.
config.rs:201 calls {data_dir}/{alias}/index/ "durable index files," but the code comment at build/rebuild.rs:100-106 says both directories hold "ephemeral, content-addressed artifacts… the canonical data lives in the CAS afterward." So this is not a durability lever — nothing is lost by the default. What it actually governs is build-time I/O location and RAM: rebuild.rs:94 falls back to std::env::temp_dir().join("fluree-index"), which on most boxes is a different filesystem from the ledger dir and on many Linux hosts is tmpfs, i.e. index build artifacts staged in RAM. Fast until a large build isn't.
Might be worth saying that here, since this doc comment is now the place an embedder learns what the knob is for, and "durable" vs "where the build stages" leads to different decisions.
Same for incremental_max_commits — config.rs:216-222 notes larger windows increase touched leaves and erode the incremental advantage, so it's two-sided rather than "bigger is better," and incremental-vs-full is the largest single cost step in the indexer.
There was a problem hiding this comment.
Folded in almost verbatim (e1b753c9d), because your framing was simply better than the config doc's: the doc now says outright that data_dir is not a durability lever (ephemeral content-addressed staging; the canonical index lives in the CAS; nothing is lost by the default), that what it actually governs is build-time I/O location and RAM — with the temp_dir fallback being a different filesystem and often tmpfs on Linux, i.e. "fast until a large build isn't" — and that incremental_max_commits is two-sided per config.rs, larger windows touching more leaves and eroding the incremental advantage. We verified all three of your references (config.rs's "durable" oversell, the rebuild.rs temp_dir fallback, and the ephemeral-artifacts comment) before rewording.
| /// | ||
| /// Takes effect on the build paths that start an in-process indexer: | ||
| /// [`build`], `build_s3*`, and the encrypted variants. It is a silent | ||
| /// no-op under [`build_memory`] (background indexing is unconditionally |
There was a problem hiding this comment.
Documenting the no-op is the right call, but a tracing::debug! (or warn!) when a config is supplied and then discarded would cost nothing and save someone an afternoon — build_memory() is the most likely first thing an embedder reaches for, and a silently ignored data_dir or incremental_max_commits looks exactly like the knob not working.
There was a problem hiding this comment.
Done (e1b753c9d), with one subtlety worth noting since it's the difference between a useful warning and a crying-wolf one: FlureeBuilder::file() pre-populates indexing_config, so keying the warn on .is_some() would fire spuriously for every build_with off a file builder. There's now a private indexer_config_user_set flag toggled only by the explicit setters (with_indexer_config / with_indexing_thresholds / with_indexing) and reset by without_indexing — an explicit opt-out shouldn't nag. The warn fires in build_memory, build_memory_encrypted, and build_with, names the build path, and is precise that novelty thresholds still apply as backpressure on every path — what's discarded is the in-process worker and the IndexerConfig knobs. Three unit tests pin it: explicit-config-into-build_memory warns; default file-builder-into-build_with stays silent; without_indexing resets.
Review follow-up on the index-install instrumentation:
- The install-lock span opened before `state.write().await`, so under
contention its duration aggregated acquisition-wait plus hold — and the
two point at opposite fixes (backpressure/install batching vs killing
the duplicated novelty walk). Replace the single span + in-lock seam
event with `index_install_wait` instrumenting exactly the
lock-acquisition future and `index_install_lock` instrumenting exactly
the guarded section, so each aggregates independently with no per-trace
arithmetic.
- Delete the `debug!("install lock acquired")` seam event the split
obsoletes. An event is a full subscriber dispatch, and this one fired
inside the exclusive lock every committer blocks on: with a fmt layer
attached, `on_event` formats and then `write_all`s through the shared
writer — a writer lock and possibly a blocking write inside the very
section the span exists to measure. Span lifecycle, by contrast, does
no writer I/O under fmt's default `FmtSpan::NONE`.
- Rename the outer span `apply_index_v2` -> `index_install`. Span names
are dashboard and saved-query dimensions; a name that embeds the format
version renames the operation the day a v3 lands. Function names keep
the version.
Per the tracing checklist (docs/contributing/tracing-guide.md): span
tree documented in docs/operations/telemetry.md, and acceptance
coverage added as AC-10 in it_tracing_spans.rs — existence, DEBUG
level, and close of all three spans; ledger_id + recorded index_t on
the root; parentage of both children under `index_install`; absence of
the old names and the in-lock event; and a second commit+install cycle
under an INFO-only subscriber capturing none of them. The checklist's
`.claude/skills/*` span-hierarchy/trace_common copies are not in this
repository (dangling references inherited with the guide), so the
telemetry.md tree and the acceptance test are the satisfiable set.
`build_memory()`, `build_memory_encrypted()`, and `build_with()`
construct with background indexing disabled, so an `IndexerConfig`
supplied through the explicit setters used to vanish silently — an
ignored `data_dir` or `incremental_max_commits` looks exactly like the
knob not working.
Key the warning on a new private `indexer_config_user_set` flag toggled
only by the explicit setters (`with_indexing`,
`with_indexing_thresholds`, `with_indexer_config`) and cleared by
`without_indexing()`. Persistent builders pre-populate
`indexing_config`, so keying on `.is_some()` would warn on every
defaulted `file()` builder routed through these paths; only an explicit
setter call makes the discard surprising. The message is scoped
precisely: novelty thresholds still flow through `derive_indexing()` as
commit backpressure on every build path — what these paths discard is
the in-process worker and the `IndexerConfig` knobs.
Also sharpen the `with_indexer_config` docs where the inherited config
doc misleads in both directions: `data_dir` is not a durability lever
(both directories hold ephemeral content-addressed build artifacts; the
canonical index lives in the CAS) — it governs where builds stage their
I/O, and the `{temp_dir}/fluree-index` fallback is a different
filesystem and often tmpfs on Linux, i.e. staged in RAM, fast until a
large build isn't; `incremental_max_commits` is two-sided rather than
bigger-is-better (larger windows touch more leaves and erode the
incremental advantage). Same correction to the "brief atomic swap"
claim on `apply_index_v2`'s rustdoc in the previous commit.
Tests pin both directions and the reset: explicit config into
`build_memory()` warns and names the build path; a default `file()`
builder through `build_with()` stays silent; an explicit
`without_indexing()` after an explicit config clears the warning.
|
All four items are in — details on each thread. The short version: the wait/hold split landed exactly as you sketched it (and your observer-effect concern about the in-lock event checks out against tracing-subscriber's actual source, so the event is gone, not just moved); the spans are renamed |
2 participants
Closes #1709.
Three small, purely-additive changes — none alters default behavior:
obs(api): spans aroundapply_index_v2and its install-lock section (fluree-db-api/src/ledger_manager.rs). Adebug_span!("apply_index_v2", ledger_id, index_t)around the function and a nesteddebug_span!("apply_index_install_lock")covering exactly the exclusive-write-lock section. This is the moment that blocks concurrent committers (same locklock_for_write()takes), and it was the one segment of the write path with no timing at all — the issue has the details on why "commit p99 spiked" and "an index installed" are currently un-joinable facts. Debug-level, so zero cost without a subscriber listening.feat(api):FlureeBuilder::with_indexer_config(IndexerConfig). Merges with any thresholds already set the same waywith_indexing_thresholdspreserves an existingIndexerConfig— the two compose in either order. Unlocksincremental_max_commits,run_budget_bytes, and (notably)data_dirfor embedded users without routing them through the escape hatches that silently drop theLedgerManager.feat(api):Fluree::indexer_handle()accessor, so embedded users can reachstatus()/wait_for_idle()instead of pollingindex_t(which observes installs but not builds, and can't distinguish idle from wedged).cargo check --all-targets,cargo clippy --all-targets --no-deps -- -D warnings, andcargo fmtall green on-p fluree-db-api.One scope note, same as in the issue: the span in (1) is observability only — the duplicated novelty walk inside the install section (state built by
apply_loaded_dband then rebuilt byreseed_runtime_small_dicts) looks like a cheap win but is a behavior change I deliberately kept out of an instrumentation PR. Once this span produces real numbers on the lock-hold cost, that's easy to argue on its own merits.Post-review addendum (commits
e42602cf5,e1b753c9d): the install span is split intoindex_install_wait/index_install_lockunder a renamedindex_installouter span (wait and hold now aggregate independently; the in-lock debug event is deleted — its dispatch mechanism was verified against tracing-subscriber source and it really was a writer-lock + potential blocking write inside the critical section). Telemetry span tree and a full acceptance test (AC-10) added per the repo's tracing checklist — with the discovery, discussed on the review thread, that the wider six-file obligation belongs todb-rand this repo carries dangling references to skills it has never had.with_indexer_configdocs now state precisely whatdata_dirgoverns (build-time I/O + RAM staging, not durability) and thatincremental_max_commitsis two-sided; explicitly-supplied indexer configs that a build path discards nowwarn!, with an explicit-set flag sofile()'s defaults never false-positive, tested in both directions.