Skip to content

refactor: share the metrics helpers, error type, and prefetch observation - #5

Open
ctxswitch wants to merge 1 commit into
cleanup/phase-3-deletionsfrom
cleanup/phase-4a-metrics-errors
Open

refactor: share the metrics helpers, error type, and prefetch observation#5
ctxswitch wants to merge 1 commit into
cleanup/phase-3-deletionsfrom
cleanup/phase-4a-metrics-errors

Conversation

@ctxswitch

Copy link
Copy Markdown
Owner

Summary

Fourth of six phases, split in two to keep the diff reviewable. This is 4a, based on
cleanup/phase-3-deletions (#4).
4b follows with the axum extractor, the negotiator
merge, and a list of smaller consolidations. Net −167 lines.

Three duplications that had already drifted:

Metrics plumbing. The agent kept its own copy of the telemetry registration
helpers — agent_register/agent_counter/agent_gauge/agent_histogram had the
same bodies as their telemetry counterparts, transfer_buckets was
character-identical, saturating_i64 differed only in usize vs u64, and both
encode methods were the same line. The telemetry helpers are now pub(crate) and
the copies are gone. Full metric names are passed at the call site instead of
formatting a flywheel_agent_ prefix nineteen times at startup, AgentMetrics::new
is a struct literal, flywheel_requests_total goes through int_counter instead of
being hand-rolled, and a new int_counter_vec helper covers the two IntCounterVecs.

ChannelStoreErrorMetadataError. The two enums differed only by
IncompatibleStore vs AlreadyExists, which forced four duplicated helper pairs in
rocksdb.rs. MetadataError gains AlreadyExists; ChannelStoreError, its
re-export, and the four channel_* helpers are deleted.

Prefetch observation. The agent's and the shard's are now one
PrefetchObservation<R: PrefetchRecorder> parameterized by its recorder, with
observe_prefetch_body — the one genuinely byte-for-byte duplicate — shared.
complete takes &mut self, which removes the Option from the unfold state and
both expects from the per-chunk path.

Verification

make ci clean, including clippy --all-targets -- -D warnings; 138 tests across the
lib and 8 integration binaries.

The exported metric surface is byte-identical. AgentMetrics::encode(3,1) and a
populated Metrics::encode() were dumped before and after the change and diffed
clean — every metric name, help string, bucket boundary, and label set. No
docs/operations.md follow-up is needed. That harness was temporary and removed
rather than left behind, since as a test it would only re-assert its own constants.

The error merge is compiler-checked, with tests/integration/channels.rs (13 tests,
both route forms, protected and open access, every data route) and the end-to-end
prefetch path in tests/integration/build_cache.rs covering runtime behaviour.

  • make ci
  • Documentation and examples are accurate
  • New or changed behavior has test coverage

Operational impact

No metric name, help text, bucket, or label changed — verified by the scrape diff
above. flywheel_requests_total is retained as a documented operator contract.

Two error display strings changed, an unavoidable consequence of the approved merge,
since the surviving variants keep the metadata wording: "channel registry failed: {0}""metadata store failed: {0}", and "channel registry task failed: {0}"
"metadata task failed: {0}". "channel already exists" is preserved verbatim.
Nothing in src/, tests/, or docs/ asserts or documents these strings, but they
are operator-visible in logs.

Two agent debug log lines lose their agent prefix ("agent prefetch request started/finished""prefetch request started/finished") and gain an empty route
field. The agent's request span already carries component = "agent" and the agent
and shard are separate processes, so nothing becomes ambiguous — flagged only in case
log-line greps exist outside this repo.

Review notes

The two prefetch observations were not actually identical, contrary to the plan.
Three real divergences, all preserved rather than papered over:

  1. The shard's has a route: String; the agent's has none. The shared type takes
    Option<String> and the agent passes None — it forwards by ring position and
    never matches a route.
  2. The shard's set_status also records the hit/miss/unavailable label. The
    agent has no equivalent because it classifies prefetch outcomes at the forwarding
    site, where it can also see the empty-ring and send-failure cases that produce no
    response at all. Folding that into the observation would have changed counts — a
    degraded 404 from an empty ring would be recorded as miss instead of
    unavailable. So AgentMetrics::prefetch_response is a deliberate no-op with a
    comment saying why. This is the one wart in the PR: a trait method that does
    nothing for one of its two impls. The alternative was changing what the counters
    mean, which seemed clearly worse.
  3. The Drop bodies differed — labeled vec counters on the shard, separate
    prefetch_completed/prefetch_cancelled counters on the agent. That is what the
    trait method absorbs.

Deliberately left alone: Metrics::new still uses sequential let bindings rather
than a struct literal, because foreground_limit is registered, .set() immediately,
and never stored as a field — forcing the literal would read worse. rocksdb.rs now
has three impl RocksMetadata blocks that could be one, the third having existed only
to group the channel methods behind the removed error type; cosmetic, left for
whoever wants it.

…tion

The agent kept its own copy of the telemetry registration helpers: agent_register
/agent_counter/agent_gauge/agent_histogram had the same bodies as their telemetry
counterparts, transfer_buckets was character-identical, saturating_i64 differed
only in its integer type, and both encode methods were the same line. The
telemetry helpers are now pub(crate) and the copies are gone, with full metric
names passed at the call site instead of formatting a flywheel_agent_ prefix
nineteen times at startup. AgentMetrics::new is a struct literal,
flywheel_requests_total goes through int_counter rather than being hand-rolled,
and a new int_counter_vec helper covers the two IntCounterVecs.

ChannelStoreError differed from MetadataError only by IncompatibleStore versus
AlreadyExists, which forced four duplicated helper pairs in rocksdb.rs.
MetadataError gains AlreadyExists and ChannelStoreError is deleted along with its
re-export and the four channel_* helpers.

The agent and shard prefetch observations are now one PrefetchObservation
parameterized by its recorder. complete takes &mut self, which removes the Option
from the unfold state and both expects from the per-chunk path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant