124 lines
6.8 KiB
Markdown
124 lines
6.8 KiB
Markdown
# テスト妥当性レビュー: session-metrics
|
|
- 評価: 概ね良い
|
|
|
|
## 確認範囲
|
|
|
|
- 対象 crate: `crates/session-metrics`
|
|
- 読んだ主なファイル:
|
|
- `crates/session-metrics/src/lib.rs`
|
|
- `crates/session-metrics/README.md`
|
|
- 関連する実利用側テスト: `crates/pod/tests/session_metrics_test.rs`
|
|
- crate の責務:
|
|
- `LogEntry::Extension { domain: "metrics" }` 上に載せる軽量な metrics 型を定義する
|
|
- `Metric` の serde 表現と builder helper を提供する
|
|
- `record_metric` で `session-store::save_extension` に metrics domain として append する
|
|
- `RestoredState.extensions` 相当の `(domain, payload)` 列から metrics domain の `Metric` だけを抽出する
|
|
|
|
## 現在のテストがよくカバーしていること
|
|
|
|
`session-metrics` 自体の unit test は 4 件で、薄い crate の主要な純粋ロジックはおおむね押さえています。
|
|
|
|
- `metric_round_trip_via_json`
|
|
- `Metric::now(...)`
|
|
- `with_dimension`
|
|
- `with_value`
|
|
- `with_correlation_id`
|
|
- JSON round-trip 後の等価性
|
|
を確認している。
|
|
- `metric_serializes_minimal_form_compactly`
|
|
- 空の `dimensions`、`None` の `value` / `correlation_id` が JSON に出ないことを確認している。
|
|
- session log に載せる payload を不要に膨らませない invariant の確認として妥当。
|
|
- `fold_skips_other_domains`
|
|
- `metrics_from_extensions` が `DOMAIN == "metrics"` の entry だけを拾うことを確認している。
|
|
- extension domain の分離という crate 境界に合っている。
|
|
- `fold_skips_undeserializable_payloads`
|
|
- metrics domain でも deserialize できない payload を落として継続することを確認している。
|
|
- コメントにある「schema 変更で読めない payload は無視する」方針に対応している。
|
|
|
|
加えて、`pod` crate 側の `crates/pod/tests/session_metrics_test.rs` が実利用パスをかなりよく補っています。
|
|
|
|
- prune metrics の `skip` / `fire` / `post_request` が session log に残り、`metrics_from_extensions` で復元できる。
|
|
- `correlation_id` による `prune.fire` と `prune.post_request` の join 前提を確認している。
|
|
- `record_metric` 経由の metrics write failure が本体 run を abort しないことを確認している。
|
|
- metrics 以前の old session が clean に replay できることを確認している。
|
|
|
|
このため、crate 単体テストは小さいものの、利用側の integration coverage まで含めると現在の責務に対する妥当性は高めです。
|
|
|
|
## 不足 / 疑問のあるテスト
|
|
|
|
- `record_metric` 自体の直接テストが `session-metrics` crate 内にありません。
|
|
- 現状は `pod` 側 integration test で間接的に成功・失敗パスを踏んでいます。
|
|
- ただし、この crate の public API としては `record_metric` が `DOMAIN` を正しく使い、`StoreError` をそのまま返し、restore 後に `Metric` として読めることを直接確認するテストがある方がよいです。
|
|
- serde default の後方互換テストが片方向です。
|
|
- 現在は「空/None の field を serialization で省略する」ことは確認しています。
|
|
- しかし「省略された JSON を deserialize して `dimensions = {}`、`value = None`、`correlation_id = None` になる」ことは直接確認していません。
|
|
- `metrics_from_extensions` の順序保持は実質的に確認されていますが、意図としてはもう少し明示してもよいです。
|
|
- `fold_skips_other_domains` で `a`, `b` の順序は見ています。
|
|
- これは十分とも言えますが、session-log の時系列を壊さないことが重要ならテスト名か assertion message で invariant として明記すると読みやすいです。
|
|
- `value: Option<f64>` の非有限値の扱いは未定義に見えます。
|
|
- `serde_json` は `NaN` / infinity の扱いが直感的でないため、metrics に非有限値が入る可能性を許すなら仕様化が必要です。
|
|
- 現時点で producer が通常の token count / duration / savings だけなら優先度は低いです。
|
|
- 実装詳細に寄りすぎた脆いテストは少ないです。
|
|
- JSON の「field 名文字列を含まない」確認はやや表層的ですが、この crate では serde wire format 自体が API なので許容範囲です。
|
|
- ただし厳密な JSON 文字列全体比較ではなく `contains` 否定に留めているため、過度には脆くありません。
|
|
|
|
## 追加の提案
|
|
|
|
優先度順に追加するなら以下です。
|
|
|
|
1. `record_metric_appends_metrics_extension_and_restores`
|
|
- `FsStore` か軽い fake `Store` を使う。
|
|
- `record_metric(...)` を呼ぶ。
|
|
- `session_store::restore(...)` 後に `state.extensions` から `metrics_from_extensions(...)` して、元の `Metric` が 1 件取れることを確認する。
|
|
- これで `DOMAIN`、payload serde、session-store への append path を crate 側で直接守れる。
|
|
|
|
2. `record_metric_propagates_store_error`
|
|
- append が必ず失敗する minimal fake `Store` を置く。
|
|
- `record_metric` が `Err(StoreError)` を返すことを確認する。
|
|
- `pod` 側の「失敗しても run 継続」は pod の責務なので、ここでは error propagation だけで十分。
|
|
|
|
3. `metric_deserializes_minimal_legacy_payload`
|
|
- `{"name":"x","ts":1}` から `Metric` に deserialize する。
|
|
- `dimensions.is_empty()`、`value.is_none()`、`correlation_id.is_none()` を確認する。
|
|
- 現在の `#[serde(default)]` / optional field の後方互換意図を直接固定できる。
|
|
|
|
4. 必要なら `metric_non_finite_value_behavior_is_documented`
|
|
- 非有限値を禁止するなら constructor/helper 側で弾く設計にしてからテスト。
|
|
- 許すなら JSON payload 上でどう表現されるかを明示する。
|
|
- 現時点では必須ではなく、producer が非有限値を作る可能性が出た時点でよいです。
|
|
|
|
## 実行したコマンド
|
|
|
|
```sh
|
|
cd /home/hare/Projects/yoi && cargo test -p session-metrics
|
|
```
|
|
|
|
結果:
|
|
|
|
```text
|
|
running 4 tests
|
|
test tests::fold_skips_other_domains ... ok
|
|
test tests::fold_skips_undeserializable_payloads ... ok
|
|
test tests::metric_round_trip_via_json ... ok
|
|
test tests::metric_serializes_minimal_form_compactly ... ok
|
|
|
|
test result: ok. 4 passed; 0 failed
|
|
```
|
|
|
|
```sh
|
|
cd /home/hare/Projects/yoi && cargo test -p pod --test session_metrics_test
|
|
```
|
|
|
|
結果:
|
|
|
|
```text
|
|
running 5 tests
|
|
test old_sessions_without_metrics_replay_cleanly ... ok
|
|
test metric_write_failure_emits_warn_alert_and_does_not_abort_run ... ok
|
|
test prune_metrics_record_below_min_savings_skip ... ok
|
|
test prune_metrics_emit_skip_then_fire_with_post_request_join ... ok
|
|
test prune_metrics_fire_during_single_long_task_without_multiple_user_turns ... ok
|
|
|
|
test result: ok. 5 passed; 0 failed
|
|
|