Skip to content

fix(trace): say what the metrics flag actually means, everywhere - #83

Merged
htcom-code merged 2 commits into
mainfrom
fix/trace-flag-claims
Sep 21, 2026
Merged

htcom-code merged 2 commits into
mainfrom
fix/trace-flag-claims

Conversation

@htcom-code

Copy link
Copy Markdown
Owner

⚠️ Stacked on #82 — base branch is chore/dev-version-next-snapshot, not
main. Merge #82 first; GitHub retargets this one when that branch is deleted.
The two touch the same paragraphs of 04-rest-api.md and StreamReady, so they
could not be made independent (rebasing this onto main conflicts).

What & why

Two claims about disabled metrics were false, and the console found both by
connecting a real client to a real server. Neither was reachable by the test
suite as it stood.

"Empty when disabled" was never true

TraceMetrics.snapshots() iterates byModule with no reference to the flag. The
flag gates observe, so turning it off stops new observations and keeps
everything already aggregated.

A reverse grep for that claim found it in four places, not one — the method's
own javadoc, the REST controller's, and, worse, the published guide in both
languages, where a consumer reads it:

docs/guide/04-rest-api.md:370
  "...when off, it returns an empty list."     ← shipped, and false

An empty result means "nothing was ever aggregated", never "aggregation is
off"
, and all four now say so. The lesson is the scope rather than the rule: when
a comment is wrong, assume the claim has already been copied.

The ready ack disagreed with the MCP tool under the same field name

TraceMetrics.enabled() is this platform's answer to "are metrics on", and it
already folds in trace.enabled, because recording is gated before aggregation is
reached. ModuleMetricsTool reports that value. The ack added in #80 reached past
it for the raw switch:

// before
StreamReady.of(trace.isEnabled(), trace.getMetrics().isEnabled(), ...)
//                                ^ raw switch, not metrics.enabled()

So the same question had two answers depending on which surface asked it. The ack
now reports the effective value, and metricsEnabled is defined in the contract
as the conjunction
rather than described by its value — an implementation
sending the raw switch is not conforming. Wording that only said "reports whether
metrics are on" would have left that open, and three implementations share this
contract.

The fix removes one wrong answer and creates another, so the docs say that too

{tracesEnabled: false, metricsEnabled: true} can no longer be reported. But on
{false, false} a client reading only metricsEnabled will offer to turn metrics
on, and turning them on changes nothing while recording is off. The field answers
"are metrics accruing", never "would enabling them help" — so tracesEnabled
still has to be read first. The console raised this after the first correction and
it is the sharper statement of the limit.

Type of change

  • fix — bug fix (no new API)
  • feat — new capability (dynamic loading, gates, isolation, MCP, debug, data access)
  • perf — performance, no observable behaviour change
  • docs — documentation only
  • refactor / chore / test / ci — no behaviour change

The Java API is unchanged; what changes is the value one SSE field carries.

Correctness & safety

Confined to the observability read path — no ClassLoader, lifecycle or gate code
is touched.

  • N/A — Unload path: not touched. Nothing here creates, holds or releases a ClassLoader.
  • Added/updated exception-case tests — three, each failing on the specific
    regression rather than on shape (see Verification).
  • N/A — Promotion gates: not touched.
  • Config gate keys verified through the binding path — TraceDisabledGatesMetricsTest
    boots a real context with trace.enabled=false + trace.metrics.enabled=true
    and asserts against the bound ProteanProperties, not direct injection.

Verification

  • gradle clean bootJar assembles
  • gradle test passes (full regression, run after the jar exists) — 599
    tests, 0 failures, 0 skipped
  • Docs updated — docs/guide/04-rest-api.md + .ko.md
  • Container/isolation tests actually ran (not self-skipped)
  • PoC gate passes — 6 combinations x 6 criteria, no SKIPPED, no unreported cell

Three tests, each failing on the specific regression:

  1. retention across a disable — observe while on, flip the live key off, assert
    the row survives and the count does not grow.
  2. no accrual while recording is off although the switch is on — requests are
    served, nothing aggregates.
  3. the ack carries the effective value — runs in the configuration where the
    two readings diverge, so a change back to the raw switch fails here instead of
    shipping.

The third matters because the suite runs from exploded classes and cannot
otherwise observe this frame's honesty — the same blind spot that let the version
in #82 go unchecked.

Base automatically changed from chore/dev-version-next-snapshot to main September 21, 2026 07:15
Two claims about disabled metrics were false, and the console found both by
connecting a real client to a real server. Neither was reachable by the test
suite as it stood.

**"Empty when disabled" was never true.** `TraceMetrics.snapshots()` iterates
`byModule` with no reference to the flag; the flag gates `observe`, so turning it
off stops new observations and keeps everything already aggregated. A reverse grep
for that claim found it in four places, not one: the method's own javadoc, the
REST controller's, and -- worse -- the published guide in both languages, where a
consumer reads it. An empty result means "nothing was ever aggregated", never
"aggregation is off", and all four now say so.

**The `ready` ack disagreed with the MCP tool under the same field name.**
`TraceMetrics.enabled()` is this platform's answer to "are metrics on" and it
already folds in `trace.enabled`, because recording is gated before aggregation is
reached. `ModuleMetricsTool` reports that value. The ack I wrote reached past it
for the raw switch, so the same question had two answers depending on which
surface asked it. The ack now reports the effective value, and `metricsEnabled` is
defined in the contract as the conjunction rather than described by its value --
an implementation sending the raw switch is not conforming, which a wording that
only said "reports whether metrics are on" would have left open.

That fix removes one wrong answer and creates another, so the docs say that too:
`{tracesEnabled: false, metricsEnabled: true}` can no longer be reported, but on
`{false, false}` a client reading only `metricsEnabled` will offer to turn metrics
on, and turning them on changes nothing while recording is off. The field answers
"are metrics accruing", never "would enabling them help". The console raised this
after the first correction and it is the sharper statement of the limit.

Three tests, each failing on the specific regression rather than on shape:
retention across a disable, no accrual while recording is off although the switch
is on, and the ack carrying the effective value. The last one runs in the
configuration where the two readings diverge, so a change back to the raw switch
fails instead of shipping -- which matters because the test suite runs from
exploded classes and cannot otherwise observe this frame's honesty.

Verified: 599 tests, 0 failures, 0 skipped; PoC gate passes 6 combinations x 6
criteria with no SKIPPED and no unreported cell.

Co-Authored-By: htjulia <htjulia1@gmail.com>
@htcom-code
htcom-code force-pushed the fix/trace-flag-claims branch from 37b1534 to 1f22a11 Compare September 21, 2026 07:16
The `ready` ack calls `tickMs` "the push period", and a client sizing a silence
watchdog from it will read that as "a frame arrives every second". The ticker is
scheduled with a fixed *delay*, so a cycle starts a second after the previous one
ends -- and a cycle walks every module, recomputes the summary window over the
whole ring, and serializes four frames to each connected client in turn. The
effective interval stretches under exactly the load that makes a watchdog matter.

So the number is a floor. A client that treats it as exact reports a healthy
server as stalled, which is the failure the field was added to prevent.

Found by the console session while consuming this contract: it sizes its stale
timeout at six ticks and has room, but noticed that nothing in the contract said
the value was a minimum.

Co-Authored-By: htjulia <htjulia1@gmail.com>
@htcom-code
htcom-code merged commit 78f75f8 into main Sep 21, 2026
7 checks passed
@htcom-code
htcom-code deleted the fix/trace-flag-claims branch September 21, 2026 07:27
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