Skip to content

fix: include custom metrics in cog predict --json output#2897

Merged
markphelps merged 2 commits intomainfrom
fix/custom-metrics-cli-response
Apr 1, 2026
Merged

fix: include custom metrics in cog predict --json output#2897
markphelps merged 2 commits intomainfrom
fix/custom-metrics-cli-response

Conversation

@markphelps
Copy link
Copy Markdown
Contributor

The Go Response struct was missing a Metrics field, causing json.Decoder to silently discard custom metrics from the coglet HTTP response. Add the field and an integration test that exercises the full CLI path (record_metric -> coglet -> Go CLI -> JSON output).

The Go Response struct was missing a Metrics field, causing
json.Decoder to silently discard custom metrics from the coglet
HTTP response. Add the field and an integration test that exercises
the full CLI path (record_metric -> coglet -> Go CLI -> JSON output).
@markphelps markphelps requested a review from a team as a code owner March 31, 2026 19:59
@ask-bonk
Copy link
Copy Markdown

ask-bonk bot commented Mar 31, 2026

Now I understand the full picture. The PR adds:

  1. Metrics field to Response struct - This fixes the bug where custom metrics were being silently dropped during JSON decoding
  2. Bonus improvements to GetSchema() - Better context handling and fixes a resource leak by adding defer resp.Body.Close()
  3. Integration test - Verifies the end-to-end flow works correctly

All changes look correct:

  • The omitempty tag maintains backward compatibility
  • map[string]any is appropriate for arbitrary metrics
  • The test exercises the full path from record_metric() → coglet → Go CLI → JSON output
  • Resource leak in GetSchema() is fixed

LGTM

github run

@ask-bonk
Copy link
Copy Markdown

ask-bonk bot commented Mar 31, 2026

LGTM

github run

@markphelps markphelps added this pull request to the merge queue Apr 1, 2026
Merged via the queue into main with commit f3eaa07 Apr 1, 2026
35 checks passed
@markphelps markphelps deleted the fix/custom-metrics-cli-response branch April 1, 2026 13:17
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.

2 participants