diff --git a/docs/guide/04-rest-api.ko.md b/docs/guide/04-rest-api.ko.md index 2b81361..be9be98 100644 --- a/docs/guide/04-rest-api.ko.md +++ b/docs/guide/04-rest-api.ko.md @@ -367,7 +367,9 @@ POST /platform/modules/cp-mod/approve?approver=alice ### GET `/platform/traces/metrics` -모듈별 집계 메트릭(요청 수·오류율·지연 백분위)을 조회한다. **opt-in** — `protean.trace.metrics.enabled=true` 일 때만 집계되며, 꺼져 있으면 빈 목록을 반환한다. +모듈별 집계 메트릭(요청 수·오류율·지연 백분위)을 조회한다. **opt-in** — `protean.trace.metrics.enabled=true` 일 때만 집계된다. + +그 플래그를 끈다고 이 엔드포인트가 비지는 **않는다**. 플래그가 막는 것은 집계이지 노출이 아니므로, 끄기 전에 관찰된 행은 그대로 계속 나간다. 따라서 빈 목록은 *"집계된 적이 없다"* 는 뜻이지 *"집계가 꺼져 있다"* 가 아니다 — 둘을 구별하려면 SSE `ready` 프레임의 `metricsEnabled` 를 읽는다. - 쿼리 파라미터: - `moduleId`(선택) — 지정 시 그 모듈만(추적 안 된 모듈이면 빈 목록), 생략 시 추적 중인 전 모듈. @@ -399,7 +401,7 @@ POST /platform/modules/cp-mod/approve?approver=alice - named 이벤트: - `ready` — 연결 확인(ack). 언제나 첫 프레임이다(아래 참고) - `trace` — 새 `RequestTrace` 델타 - - `metrics` — `ModuleMetricsSnapshot[]` 갱신(모듈별 누적 집계, `protean.trace.metrics.enabled=true` 일 때만 채워짐) + - `metrics` — `ModuleMetricsSnapshot[]` 갱신(모듈별 누적 집계. 새 관찰은 `protean.trace.metrics.enabled=true` 일 때만 쌓이지만, 끄기 전에 집계된 행은 계속 나간다) - `modules` — 현재 `ModuleStatus[]` - `summary` — `TraceSummary`: 콘솔 헤더용 **윈도** 집계(아래 참고) - `/platform/traces` 와 마찬가지로 스트림 연결 자체는 trace 기록에서 제외된다(자기-관측 방지). @@ -416,9 +418,14 @@ data: {"platform":"protean","platformVersion":"0.1.0","tracesEnabled":true, - `platform` — 구현체 식별자. 언제나 `"protean"`. 이 계약을 여러 구현이 공유하므로, 버전 숫자만 받은 콘솔이 그것이 누구의 버전인지 추측하지 않도록 함께 싣는다. **표시·진단용이며 동작을 분기하는 데 쓰지 않는다.** - `platformVersion` — 이 라이브러리의 버전. jar manifest 의 `Implementation-Version` 에서 읽는다. manifest 가 없는 레이아웃(exploded classes — 이 라이브러리 자신의 테스트, 그리고 클래스 디렉터리로 잡힌 IDE 실행)에서는 **`null`** 이다. 그 상황에서는 버전을 정말로 모르며, 임의의 대체 문자열은 "지금 어느 버전과 이야기하고 있나" 에 대한 지어낸 답이 된다. 실제로는 보기 드물다 — 소비자 앱은 protean 을 jar 로 해석하고 `examples/quickstart` 데모도 그러므로, **콘솔이 실제로 붙는 경로는 전부 진짜 버전을 내려보낸다.** 필드 자체는 항상 있으므로 클라이언트가 "없음" 과 "모름" 을 구별할 일이 없다. -- `tracesEnabled` / `metricsEnabled` — `protean.trace.enabled` 와 `protean.trace.metrics.enabled`. **"기록이 꺼져 있다"** 와 **"켜져 있는데 아직 아무 일도 없었다"** 를 가른다. 빈 스트림만으로는 이 둘이 구별되지 않는다. +- `tracesEnabled` — `protean.trace.enabled`. **"기록이 꺼져 있다"** 와 **"켜져 있는데 아직 아무 일도 없었다"** 를 가른다 — 빈 스트림만으로는 구별되지 않는다. +- `metricsEnabled` — **`protean.trace.enabled` AND `protean.trace.metrics.enabled` 로 정의된다.** metrics 스위치 단독 값이 아니다. 기록은 집계보다 **앞서** 막히므로 `tracesEnabled` 가 false 인 동안 그 스위치는 아무 의미가 없다. MCP 메트릭 툴이 같은 이름으로 답하는 값과 같으며, 이 계약을 구현하면서 원시 스위치를 싣는 것은 계약 위반이다. + + ⚠️ **그래도 `tracesEnabled` 를 먼저 읽어야 한다.** 논리곱은 틀린 답 하나를 없애지만(`{"tracesEnabled": false, "metricsEnabled": true}` 는 이 프레임이 내보낼 수 없는 상태다) 다른 하나를 만든다. `{"tracesEnabled": false, "metricsEnabled": false}` 에서 `metricsEnabled` 만 보는 클라이언트는 *"메트릭이 꺼져 있다 — 켜면 행이 생긴다"* 고 말하는데, 기록이 꺼져 있는 한 켜도 아무 일도 일어나지 않는다. `metricsEnabled` 는 "메트릭이 쌓이고 있는가" 에 답할 뿐 **"켜면 도움이 되는가" 에는 답하지 않는다.** - `buffered` — 이 프레임 **다음에 오는 `trace` 프레임이 싣는 행 수**이며, 링이 보유한 행 수가 아니다. `protean.trace.capacity` 가 재생 상한 200 을 넘으면 둘이 달라진다. 숫자만 알리고 행은 주지 않는 것은 둘 다 안 하느니만 못하다. -- `tickMs` — 푸시 주기(`1000`). 클라이언트의 침묵 감지 워치독이 가정 대신 계약을 기준으로 자기 값을 정할 수 있게 한다. +- `tickMs` — 푸시 간격(`1000`). 클라이언트의 침묵 감지 워치독이 가정 대신 계약을 기준으로 자기 값을 정할 수 있게 한다. + + ⚠️ **이 값은 하한이지 보장이 아니다.** 티커는 *fixed delay* 로 걸려 있어 다음 주기는 이전 주기가 **끝난** 뒤 이만큼 지나 시작한다 — 그리고 한 주기는 모듈을 전수 조회하고, 링 전체로 윈도를 다시 계산하고, 연결된 클라이언트마다 차례로 네 프레임을 직렬화해 보낸다. 부하가 커지면 실효 간격이 늘어난다. 워치독은 `tickMs` 의 **넉넉한 배수**로 잡아야 한다 — 정확한 주기로 믿는 클라이언트는 멀쩡한 서버를 멈춘 것으로 보고하게 된다. - `capacity` — **이 연결 시점의** `protean.trace.capacity`. 클라이언트가 "이보다 과거는 서버에 없다" 를 말할 수 있다. LIVE 키이므로 이후 변경은 이미 연결된 클라이언트에 닿지 않으며, 재연결 때 새 값이 다시 나간다. 이후 버전에서 `ready` 에 필드가 더해질 수 있다 — 클라이언트는 모르는 필드를 무시해야 한다. diff --git a/docs/guide/04-rest-api.md b/docs/guide/04-rest-api.md index 24e80d7..6860a7d 100644 --- a/docs/guide/04-rest-api.md +++ b/docs/guide/04-rest-api.md @@ -367,7 +367,9 @@ Query recent request-execution traces, newest-first. If `protean.trace.enabled=f ### GET `/platform/traces/metrics` -Query per-module aggregate metrics (request count, error rate, latency percentiles). **opt-in** — aggregation happens only when `protean.trace.metrics.enabled=true`; when off, it returns an empty list. +Query per-module aggregate metrics (request count, error rate, latency percentiles). **opt-in** — aggregation happens only when `protean.trace.metrics.enabled=true`. + +Turning that flag off does **not** empty this endpoint: it gates aggregation, not exposure, so rows observed before it was turned off keep being returned. An empty list therefore means *"nothing was ever aggregated"*, never *"aggregation is off"* — to tell those apart, read `metricsEnabled` from the SSE `ready` frame. - Query parameters: - `moduleId` (optional) — when set, only that module (empty list if the module is not tracked); when omitted, all tracked modules. @@ -399,7 +401,7 @@ Live server-sent-events (SSE) push stream — the console uses it in place of 5- - Named events: - `ready` — connection acknowledgement, always the first frame (see below) - `trace` — new `RequestTrace` deltas - - `metrics` — a `ModuleMetricsSnapshot[]` update (per-module cumulative aggregate; populated only when `protean.trace.metrics.enabled=true`) + - `metrics` — a `ModuleMetricsSnapshot[]` update (per-module cumulative aggregate; new observations accrue only when `protean.trace.metrics.enabled=true`, but rows aggregated before it was turned off keep being sent) - `modules` — the current `ModuleStatus[]` - `summary` — a `TraceSummary`: a **windowed** aggregate for the console header (see below) - Like `/platform/traces`, the stream connection itself is excluded from trace recording (no self-observation). @@ -416,9 +418,14 @@ It carries only what a client must know **once, at connect time** and cannot der - `platform` — implementation identifier, always `"protean"`. Present so a console watching several implementations of this contract is not left reading a bare version number and guessing whose it is. **Display and diagnostics only — do not branch behaviour on it.** - `platformVersion` — this library's version, read from the jar manifest (`Implementation-Version`). **`null`** when running from a layout that has no manifest (exploded classes: the library's own test suite, and IDE runs configured against class directories) — the version is genuinely unknown there, and a placeholder would be a fabricated answer. In practice it is rarely seen: a consumer application resolves protean as a jar, and so does the `examples/quickstart` demo, so every path a console actually connects to reports a real version. The field is always present, so a client never has to distinguish "absent" from "unknown". -- `tracesEnabled` / `metricsEnabled` — `protean.trace.enabled` and `protean.trace.metrics.enabled`. They separate **"recording is off"** from **"recording is on but nothing has happened yet"**, which an empty stream cannot. +- `tracesEnabled` — `protean.trace.enabled`. Separates **"recording is off"** from **"recording is on but nothing has happened yet"**, which an empty stream cannot. +- `metricsEnabled` — **defined as `protean.trace.enabled` AND `protean.trace.metrics.enabled`**, not the metrics switch on its own. Recording is gated before aggregation is reached, so the switch means nothing while `tracesEnabled` is false. This is the same value the MCP metrics tool reports under the same name; an implementation of this contract that sends the raw switch is not conforming. + + ⚠️ **Read `tracesEnabled` first anyway.** The conjunction removes one wrong answer — `{"tracesEnabled": false, "metricsEnabled": true}` is not a state this frame can report — but it creates another. On `{"tracesEnabled": false, "metricsEnabled": false}` a client that looks only at `metricsEnabled` will say *"metrics are off; turn them on and rows appear"*, and turning them on changes nothing while recording is off. `metricsEnabled` answers "are metrics accruing", never "would enabling them help". - `buffered` — how many rows the `trace` frame that follows this one carries, **not** how many the ring holds. The two differ whenever `protean.trace.capacity` exceeds the 200-row replay cap; announcing a count while withholding the rows would be worse than announcing neither. -- `tickMs` — the push period (`1000`), so a client's silence watchdog has a contract to size itself against instead of assuming one. +- `tickMs` — the push interval (`1000`), so a client's silence watchdog has a contract to size itself against instead of assuming one. + + ⚠️ **It is a floor, not a guarantee.** The ticker is scheduled with a *fixed delay*, so the next cycle starts this long after the previous one **ends** — and a cycle walks every module, recomputes the window over the ring, and serializes four frames to every connected client in turn. Under load the effective interval stretches. Size a watchdog at a generous multiple of `tickMs`; a client that treats it as an exact period will report a healthy server as stalled. - `capacity` — `protean.trace.capacity` **as of this connection**, letting a client say "nothing older than this is on the server". It is a live key, so a later change does not reach an already-connected client; the value is re-sent on reconnect. Fields may be added to `ready` in later versions — a client must ignore the ones it does not recognize. diff --git a/src/main/java/org/htcom/protean/runtime/TraceMetrics.java b/src/main/java/org/htcom/protean/runtime/TraceMetrics.java index 0e20c6b..9aa79a4 100644 --- a/src/main/java/org/htcom/protean/runtime/TraceMetrics.java +++ b/src/main/java/org/htcom/protean/runtime/TraceMetrics.java @@ -116,7 +116,13 @@ public Optional snapshot(String moduleId) { return m == null ? Optional.empty() : Optional.of(m.snapshot(key)); } - /** Snapshots of every tracked module (unordered). Empty when disabled or nothing observed yet. */ + /** + * Snapshots of every tracked module (unordered). Empty only when nothing has been observed yet — + * not when aggregation is disabled. {@code protean.trace.metrics.enabled} gates {@link #observe}, + * so turning it off stops new observations but retains what was already aggregated, and those rows keep + * being returned. Callers that need to say why a result is empty must read the flag separately rather + * than infer it from emptiness. + */ public List snapshots() { List out = new ArrayList<>(); for (Map.Entry e : byModule.entrySet()) { diff --git a/src/main/java/org/htcom/protean/web/StreamReady.java b/src/main/java/org/htcom/protean/web/StreamReady.java index a87fa8a..0c787e5 100644 --- a/src/main/java/org/htcom/protean/web/StreamReady.java +++ b/src/main/java/org/htcom/protean/web/StreamReady.java @@ -26,12 +26,22 @@ * a fabricated answer to "which version am I talking to". * @param tracesEnabled {@code protean.trace.enabled}: distinguishes "recording is off" from "recording is on * but nothing has happened yet", which an empty stream cannot. - * @param metricsEnabled {@code protean.trace.metrics.enabled}: same distinction for the {@code metrics} frame. + * @param metricsEnabled {@code protean.trace.enabled} AND {@code protean.trace.metrics.enabled}, not the + * metrics switch alone: recording is gated before aggregation is reached, so the switch + * means nothing while {@code tracesEnabled} is false. This is the same value the MCP + * metrics tool reports under the same name. It answers "are metrics accruing" and + * not "would enabling them help" — on {@code (false, false)} a client reading + * only this field would offer to turn metrics on, which changes nothing while recording + * is off, so {@code tracesEnabled} still has to be read first. * @param buffered how many rows the {@code trace} frame that follows this one carries — not how * many the ring holds. The two differ whenever the ring is larger than the replay cap, and * announcing a count while withholding the rows is worse than announcing neither. - * @param tickMs the push period, so a client's silence watchdog has a contract to size itself against - * instead of assuming one. + * @param tickMs the push interval, so a client's silence watchdog has a contract to size itself against + * instead of assuming one. It is a floor, not a guarantee: the ticker runs on a + * fixed delay, so a cycle starts this long after the previous one ends, and a + * cycle walks every module, recomputes the window over the whole ring, and serializes four + * frames to each connected client in turn. A watchdog should allow a generous multiple of + * this rather than treat it as an exact period. * @param capacity {@code protean.trace.capacity} as of this connection, letting a client say * "nothing older than this is on the server". It is a live key, so a later change does not * reach an already-connected client; the value is re-sent on reconnect. diff --git a/src/main/java/org/htcom/protean/web/TraceAdminController.java b/src/main/java/org/htcom/protean/web/TraceAdminController.java index efb3dc2..05d02d9 100644 --- a/src/main/java/org/htcom/protean/web/TraceAdminController.java +++ b/src/main/java/org/htcom/protean/web/TraceAdminController.java @@ -65,7 +65,11 @@ public List recent(@RequestParam(defaultValue = "50") int limit, /** * Per-module aggregated metrics (opt-in; {@code protean.trace.metrics.enabled}). With {@code moduleId}, * returns that module's metrics (empty list if untracked); without it, every tracked module. - * Returns an empty list when metrics are disabled. + * + *

Disabling metrics does not empty this: the flag gates aggregation, not exposure, so rows + * observed before it was turned off keep being returned. An empty list therefore means "nothing was ever + * aggregated", never "aggregation is off" — a client that needs to tell those apart reads the flag, which + * the SSE {@code ready} frame carries as {@code metricsEnabled}. */ @GetMapping("/metrics") public List metrics(@RequestParam(required = false) String moduleId) { diff --git a/src/main/java/org/htcom/protean/web/TraceStreamService.java b/src/main/java/org/htcom/protean/web/TraceStreamService.java index a63fa53..9798a64 100644 --- a/src/main/java/org/htcom/protean/web/TraceStreamService.java +++ b/src/main/java/org/htcom/protean/web/TraceStreamService.java @@ -127,8 +127,12 @@ public SseEmitter open() { */ private StreamReady ready(int buffered) { ProteanProperties.Trace trace = props.getTrace(); - return StreamReady.of(trace.isEnabled(), trace.getMetrics().isEnabled(), buffered, TICK_MS, - trace.getCapacity()); + // metrics.enabled() is the platform's own answer to "are metrics on", and it already folds in + // trace.enabled -- recording is gated before aggregation is reached, so the metrics switch alone means + // nothing while tracing is off. Reading the raw switch here would make this frame disagree with the MCP + // tool, which reports the same name from the same accessor, and would let a client promise rows that can + // never arrive. + return StreamReady.of(trace.isEnabled(), metrics.enabled(), buffered, TICK_MS, trace.getCapacity()); } /** One push cycle: advance the cursor by the newest delta, then fan the deltas + fresh snapshots out. */ diff --git a/src/test/java/org/htcom/protean/TraceDisabledGatesMetricsTest.java b/src/test/java/org/htcom/protean/TraceDisabledGatesMetricsTest.java new file mode 100644 index 0000000..1ae3d07 --- /dev/null +++ b/src/test/java/org/htcom/protean/TraceDisabledGatesMetricsTest.java @@ -0,0 +1,133 @@ +/* + * Copyright (c) 2026 htjulia + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. + */ + +package org.htcom.protean; + +import org.htcom.protean.module.ModuleDescriptor; +import org.htcom.protean.module.ModulePlatform; +import org.htcom.protean.runtime.TraceMetrics; +import org.htcom.protean.runtime.TraceStore; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; +import org.springframework.test.web.servlet.MockMvc; + +import java.nio.file.Path; +import java.util.List; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +/** + * {@code protean.trace.enabled} gates the whole recording path in {@code RequestTraceFilter}, ahead of + * per-module aggregation, so metrics accrue only while both it and {@code protean.trace.metrics.enabled} + * are true. {@link TraceMetrics#enabled()} already folds the two together, and the SSE {@code ready} ack must + * report that rather than the raw switch — otherwise the ack disagrees with the MCP metrics tool, which + * answers the same question from the same accessor, and a client acting on the ack alone would promise rows that + * can never arrive. + * + *

This runs in the configuration where the two readings diverge — the switch says on, the effective answer is + * off — so anything that goes back to reading the raw switch fails here rather than silently shipping. + */ +@SpringBootTest(properties = { + "protean.trace.enabled=false", // the outer gate + "protean.trace.metrics.enabled=true" // says "on", but nothing can reach it +}) +@AutoConfigureMockMvc +class TraceDisabledGatesMetricsTest { + + static final Path STORE_DIR = Path.of(System.getProperty("java.io.tmpdir"), "protean-trace-gate-test"); + + @DynamicPropertySource + static void props(DynamicPropertyRegistry registry) { + registry.add("protean.module-store.dir", STORE_DIR::toString); + } + + @Autowired MockMvc mockMvc; + @Autowired ModulePlatform platform; + @Autowired TraceMetrics metrics; + @Autowired TraceStore store; + @Autowired org.htcom.protean.autoconfigure.ProteanProperties props; + + static final String ID = "gate-mod"; + static final String FQCN = "runtime.gate.GateController"; + + @AfterEach + void cleanup() { + try { + platform.uninstall(ID); + } catch (RuntimeException ignored) { + } + } + + @Test + void metrics_report_off_and_never_accrue_while_trace_recording_is_off() throws Exception { + platform.install(descriptor()); + + // The switch is on, but the effective answer is off — and it is the effective answer the platform gives. + assertThat(props.getTrace().getMetrics().isEnabled()).isTrue(); + assertThat(metrics.enabled()).isFalse(); + + mockMvc.perform(get("/gate/ping")).andExpect(status().isOk()); + mockMvc.perform(get("/gate/ping")).andExpect(status().isOk()); + + // Requests are served, yet nothing is aggregated: the filter returns early on !trace.enabled, before the + // aggregation call is reached. Reporting the switch here would tell a client to expect these rows. + assertThat(metrics.snapshots()).isEmpty(); + assertThat(metrics.snapshot(ID)).isEmpty(); + // Same gate, so the ring is empty too — `buffered` in the ack is therefore 0, not a stale count. + assertThat(store.recent(200, null)).isEmpty(); + } + + @Test + void the_ready_ack_reports_the_effective_value_not_the_switch() throws Exception { + String body = mockMvc.perform(get("/platform/traces/stream") + .header("Accept", "text/event-stream")) + .andExpect(status().isOk()) + .andReturn().getResponse().getContentAsString(); + + String ready = body.lines().dropWhile(l -> !l.startsWith("event:ready")) + .filter(l -> l.startsWith("data:")).findFirst().orElseThrow(); + // Both false, although protean.trace.metrics.enabled is true: the ack answers "are metrics accruing", + // which is the same question the MCP tool answers, not "is the switch set". + assertThat(ready).contains("\"tracesEnabled\":false"); + assertThat(ready).contains("\"metricsEnabled\":false"); + } + + static ModuleDescriptor descriptor() { + String src = """ + package runtime.gate; + import org.springframework.web.bind.annotation.GetMapping; + import org.springframework.web.bind.annotation.RestController; + @RestController + public class GateController { + @GetMapping("/gate/ping") + public String ping() { return "pong"; } + } + """; + String test = """ + package runtime.gate; + import org.junit.jupiter.api.Test; + import static org.junit.jupiter.api.Assertions.assertEquals; + public class GateControllerTest { + @Test void ping() { assertEquals("pong", new GateController().ping()); } + } + """; + return ModuleDescriptor.builder() + .id(ID).version("1.0.0") + .controllerFqcn(FQCN).componentFqcns(List.of(FQCN)) + .sources(Map.of(FQCN, src)).tests(Map.of("runtime.gate.GateControllerTest", test)) + .build(); + } +} diff --git a/src/test/java/org/htcom/protean/TraceMetricsTest.java b/src/test/java/org/htcom/protean/TraceMetricsTest.java index 9afa80c..444bc47 100644 --- a/src/test/java/org/htcom/protean/TraceMetricsTest.java +++ b/src/test/java/org/htcom/protean/TraceMetricsTest.java @@ -40,6 +40,26 @@ void disabled_by_default_is_a_noop() { assertThat(m.snapshot("mod-a")).isEmpty(); } + @Test + void disabling_stops_new_observations_but_keeps_what_was_already_aggregated() { + ProteanProperties props = new ProteanProperties(); + props.getTrace().getMetrics().setEnabled(true); + TraceMetrics m = new TraceMetrics(props); + m.observe("mod-a", 200, 5, null, 1000); + + props.getTrace().getMetrics().setEnabled(false); // live key, flipped at runtime + + // The flag gates observation, not exposure. The row survives, so an empty result can never be read as + // "metrics are off" — the javadoc used to claim exactly that, and a caller believing it would report a + // disabled platform as an idle one. + assertThat(m.enabled()).isFalse(); + m.observe("mod-a", 500, 9, null, 1001); // dropped: this is what the flag actually stops + assertThat(m.snapshots()).hasSize(1); + ModuleMetricsSnapshot retained = m.snapshot("mod-a").orElseThrow(); + assertThat(retained.count()).isEqualTo(1); + assertThat(retained.errorCount()).isZero(); + } + @Test void counts_requests_and_errors() { TraceMetrics m = enabledMetrics(512);