harness(langfuse): stamp model on the ModelCompleted generation#43
Conversation
The per-call generation-create observation was projected solely from AgentEvent::ModelCompleted, which carries no model name (only ModelStarted does), so the exporter never set body["model"]. Langfuse could not map its pricing table and every agent-run generation recorded cost $0. Pre-scan the batch to correlate each call's model by call_id from ModelStarted, and stamp body["model"] on the matching ModelCompleted generation. clean_nulls drops the key when a model can't be correlated, leaving the payload unchanged for that case.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughLangfuse ingestion now correlates ChangesModel correlation for Langfuse generations
Harness cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/harness/observability/langfuse/test.rs (1)
98-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the no-correlation case.
The test validates the happy path well. Given the PR relies on
clean_nullsto dropmodelwhen noModelStartedcorrelates, a small companion test assertinggeneration["body"].get("model").is_none()for a loneModelCompletedwould lock in that fallback behavior and guard against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/harness/observability/langfuse/test.rs` around lines 98 - 142, Add a companion test in the Langfuse observability test module to cover the no-correlation fallback: use `LangfuseClient::proxy` and `build_ingestion_batch` with only a lone `AgentEvent::ModelCompleted` (no matching `ModelStarted` / `call_id` correlation), then assert the generated `generation-create` body has no `model` field via `generation["body"].get("model").is_none()`. This should sit alongside `generation_carries_model_from_model_started` and verify the `clean_nulls` path in the ingestion projection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/harness/observability/langfuse/test.rs`:
- Around line 98-142: Add a companion test in the Langfuse observability test
module to cover the no-correlation fallback: use `LangfuseClient::proxy` and
`build_ingestion_batch` with only a lone `AgentEvent::ModelCompleted` (no
matching `ModelStarted` / `call_id` correlation), then assert the generated
`generation-create` body has no `model` field via
`generation["body"].get("model").is_none()`. This should sit alongside
`generation_carries_model_from_model_started` and verify the `clean_nulls` path
in the ingestion projection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d324fc0-c930-4cdd-99af-65f2d39e0b25
📒 Files selected for processing (2)
src/harness/observability/langfuse/mod.rssrc/harness/observability/langfuse/test.rs
collapsible_if in StreamAccumulator::push_tool_chunk and a duplicated #[cfg(test)] on the stream test module both fail CI's cargo clippy --all-targets -- -D warnings under rust 1.96. Behavior-neutral.
|
CI guardian: the |
2 participants
The per-call
generation-createobservation was built solely fromAgentEvent::ModelCompleted, which carries no model name (onlyModelStarted { call_id, model }does), so the exporter never setbody["model"]. Langfuse could not map its pricing table and every agent-run generation recorded cost $0.Fix: pre-scan the batch to correlate each call's model by
call_idfromModelStarted, and stampbody["model"]on the matchingModelCompletedgeneration (AgentEvent enum unchanged).clean_nullsdrops the key when a model can't be correlated, leaving the payload unchanged for that case. Adds a test asserting the generation carries the modelModelStartedannounced.Validation:
cargo fmtclean;cargo test --libgreen (996 passed, incl. the new test). Note:cargo clippy --lib --tests -D warningsfails only on two pre-existing, unrelated lints under Rust 1.93.0 in files this PR does not touch (collapsible_ifinsrc/harness/model/mod.rs, duplicated attribute insrc/harness/stream/mod.rs); this diff introduces zero clippy warnings.Closes tinyhumansai/openhuman#4656
Summary by CodeRabbit
modelandcall_idwhen the model is supplied via a prior model-start event.