Fix rpc-messages.jsonl parsing to handle real-world event/_schema format - #53256
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates RPC telemetry parsing to support production rpc-message/v2 event fields while preserving legacy compatibility.
Changes:
- Normalizes
eventvalues across Go and JavaScript parsers. - Updates timeline, metrics, and DIFC handling with regression tests.
- Revises observability workflow guidance and recompiles its lock file.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/gateway_logs_types.go |
Adds v2 fields and type normalization. |
pkg/cli/gateway_logs_rpc.go |
Uses normalized types for metrics and tool calls. |
pkg/cli/gateway_logs_timeline.go |
Supports v2 events in timelines. |
pkg/cli/gateway_logs_timeline_rpcentry_test.go |
Tests v2 timeline conversion. |
pkg/cli/gateway_logs_test.go |
Tests v2 metrics and tool-call pairing. |
actions/setup/js/parse_mcp_gateway_log.cjs |
Normalizes v2 events in summary parsing. |
actions/setup/js/parse_mcp_gateway_log.test.cjs |
Adds JavaScript regression coverage. |
.github/workflows/daily-observability-report.md |
Documents the production schema. |
.github/workflows/daily-observability-report.lock.yml |
Updates compiled workflow metadata. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
| function getRpcMessageType(entry) { | ||
| if (typeof entry?.type === "string" && entry.type) return entry.type; | ||
| if (typeof entry?.event === "string") return RPC_EVENT_TO_TYPE[entry.event] || entry.event; | ||
| return ""; |
| - `event` or `type`: Message kind. Real-world telemetry (schema `rpc-message/v2`) uses a | ||
| top-level `event` field with values `rpc_request` / `rpc_response` (and `difc_filtered` | ||
| for blocked events); a top-level `_schema` field (e.g. `"rpc-message/v2"`) marks this |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #53256 for over-engineering per ponytail-review skill. The event/schema normalization (EffectiveType() in Go, getRpcMessageType() in JS) is a minimal, single-purpose mapping with clear precedence rules, mirrored intentionally across the two languages since Go and JS parsers don't share code. No dead code, no speculative abstractions, no unnecessary dependencies. Lean already. Ship.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
COMMENT — I didn't find a changed-line bug worth blocking on here.
What I checked
- the new
event→ legacy type normalization is applied consistently in the Go parser, tool-call reconstruction path, and timeline conversion - the JS fallback summary path now accepts
rpc-message/v2entries without mutating explicit legacytypevalues - regression coverage exists for request, response, and
difc_filteredcases in both languages
No actionable review comments this round.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 9.33 AIC · ⌖ 6.31 AIC · ⊞ 4.5K
Comment /review to run again
There was a problem hiding this comment.
The fix correctly handles real-world rpc-messages.jsonl files that use the schema rpc-message/v2 format (event field instead of legacy type field).
Go: EffectiveType() method on RPCMessageEntry cleanly normalizes both formats without mutating the struct. All call sites updated to use this method.
JS: getRpcMessageType() helper with in-place normalization of entry.type for backward-compatible downstream consumers.
Tests: Solid regression coverage in both Go and JS. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.9 AIC · ⌖ 7.02 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed:
EffectiveType()/getRpcMessageType()centralise normalisation cleanly, rather than patching every call site. - Good regression test coverage: Go and JS tests both cover
event-only entries for all three event types, andEffectiveTypehas table-driven unit tests. The issue #53254 is referenced inline. - Two minor observations (non-blocking): documented in inline comments above.
Positive Highlights
- ✅
EffectiveType()gives the Go struct a clear, narrowly-scoped interface — consistent with deep-module principles. - ✅
legacy type takes precedencetest case protects against future schema drift. - ✅ Documentation update in
daily-observability-report.mdexplicitly discourages false-positive unhealthy flags — good operational hygiene. - ✅ Both JS and Go implementations mirror each other, reducing the cognitive distance when reading across the boundary.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.2 AIC · ⌖ 9.12 AIC · ⊞ 7.7K
Comment /matt to run again
| // when absent so we never overwrite an entry's own explicit "type" value. | ||
| if (!entry.type) entry.type = messageType; | ||
|
|
||
| if (messageType === "REQUEST") { |
There was a problem hiding this comment.
[/tdd] entry.type = messageType mutates the freshly-parsed JSON object. The comment explains why this is safe, but callers who re-serialize the result or pass it to multiple consumers may be surprised.
💡 Suggestion
The comment is helpful, but consider making the safety guarantee explicit at the call site:
// entry is a freshly-parsed object from JSON.parse(trimmed); safe to mutate.
if (!entry.type) entry.type = messageType;Or avoid mutation entirely by normalizing before downstream consumers use the object:
const typedEntry = entry.type ? entry : { ...entry, type: messageType };This makes the intent clearer and prevents subtle bugs if parseRpcMessagesJsonl is ever refactored to share parsed entries.
@copilot please address this.
| @@ -301,10 +320,10 @@ function parseGatewayJsonlForDifcFiltered(jsonlContent) { | |||
| const lines = jsonlContent.split("\n"); | |||
| for (const line of lines) { | |||
| const trimmed = line.trim(); | |||
There was a problem hiding this comment.
[/diagnosing-bugs] The early-exit regex /difc_filtered/i is case-insensitive but the final check via getRpcMessageType() produces "DIFC_FILTERED" (uppercase). This works today, but the case-insensitive fast-path may silently pass lines that getRpcMessageType cannot normalize (e.g. "DIFC_Filtered" in a future schema variant).
💡 Suggestion
Make the fast-path regex and the lookup consistent. Either:
- Use a case-sensitive regex matching both known spellings:
/difc_filtered|DIFC_FILTERED/ - Or add a comment explaining the intentional loose match is just a perf guard and the inner
getRpcMessageTypecheck is the authoritative gate.
// Fast-path guard (loose): skip lines that obviously cannot be difc events.
// getRpcMessageType() below is the authoritative check.
if (!trimmed || !/difc_filtered|DIFC_FILTERED/i.test(trimmed)) continue;@copilot please address this.
Documents the decision to introduce EffectiveType()/getRpcMessageType() normalization helpers that bridge the legacy `type` field and the production schema rpc-message/v2 `event` field. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (143 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/31985733953
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Real MCP-enabled workflow runs sample
rpc-messages.jsonltelemetry with 0/202 entries exposing the top-leveltypefield the parser expected, silently zeroing out tool-call counts and durations for every run relying on this fallback (used whenevergateway.jsonlis absent — 20/20 sampled runs).Root cause
Downloaded real
rpc-messages.jsonlartifacts from recent workflow runs and confirmed: production telemetry (schemarpc-message/v2) uses a top-leveleventfield (rpc_request/rpc_response/difc_filtered) plus a_schemamarker — never the legacy top-leveltypefield (REQUEST/RESPONSE/DIFC_FILTERED) the parser, JS summary generator, and workflow docs all assumed.{"timestamp":"2026-08-15T23:48:42.233Z","event":"rpc_request","_schema":"rpc-message/v2","direction":"OUT","server_id":"github","payload":{...}}Changes
pkg/cli/gateway_logs_types.go:RPCMessageEntrygainsEvent/Schemafields and anEffectiveType()method that normalizeseventvalues to legacytypevalues, preferringtypewhen both are present (no known schema populates both with conflicting values).pkg/cli/gateway_logs_rpc.go/pkg/cli/gateway_logs_timeline.go: all directentry.Type ==comparisons replaced withentry.EffectiveType().actions/setup/js/parse_mcp_gateway_log.cjs: matchinggetRpcMessageType()helper, applied inparseRpcMessagesJsonlandparseGatewayJsonlForDifcFiltered(entry.type only backfilled when absent, to avoid clobbering existing data)..github/workflows/daily-observability-report.md: documented schema (Phase 3.4) updated to reflect the realevent/_schemaformat and explicitly instructs against flagging runs unhealthy solely for missingtype; lock file recompiled.event/_schema-only entries for REQUEST/RESPONSE/DIFC_FILTERED.