fix: correct Metrics API v2 examples#3011
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eaf44e550
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Docs/examples-only fix for Metrics API v2 — prior review feedback on response column naming, plot accessors, and the changelog rowLimit reference has been addressed in the latest commit.
Extended reasoning...
Overview
This PR is docs-only and fixes Metrics API v2 examples across four files: the v2 launch changelog, the metrics-api docs page, and the cookbook MDX + notebook source. It corrects orderBy.field to the {aggregation}_{measure} format (e.g. sum_totalCost), switches the unsupported top-level rowLimit to config.row_limit, and adds a new 'Ordering by metrics' section documenting the convention.
Security risks
None. No runtime code, auth, crypto, permissions, or shared infrastructure is touched. The diff is bounded to MDX prose, JSON example payloads, and a Python helper inside a Jupyter notebook example.
Level of scrutiny
Low. The changes are mechanical documentation corrections with no production logic and no CODEOWNER-sensitive paths. The risk of harm is limited to a reader copy-pasting a broken example.
Other factors
My prior review on commit 6eaf44e raised three concerns: (1) the numeric_suffixes helper would silently skip pd.to_numeric coercion under the newly documented {aggregation}_{measure} column format, (2) the downstream plot cell still used camelCase timeDimension and latency_p95 while the orderBy switched to snake_case time_dimension, and (3) the v2 launch changelog still advertised the unsupported top-level rowLimit. The latest commit 769b4fc addresses all three: the helper now uses startswith over aggregation prefixes (sum_, avg_, p95_, …), the plot cell uses time_dimension and p95_latency, and the changelog now says config.row_limit. A repo-wide grep confirms no remaining rowLimit references. The unchanged homepage snippet in components/home/feature-tabs/data.ts still uses totalCost_sum with view: "traces", which is internally consistent because that snippet calls the legacy v1 endpoint (api.metrics.metrics) where the response column format is {measure}_{aggregation}.
There was a problem hiding this comment.
All three of my prior review concerns were addressed — docs-only changes are internally consistent and look correct.
Extended reasoning...
Overview
This is a docs-only PR that fixes incorrect Metrics API v2 examples across the metrics docs page, the v2 launch changelog, the cookbook MDX and notebook, plus an unrelated whitespace/table-formatting cleanup in the Hermes integration page. Changes are documentation-only with no runtime/library logic touched.
Security risks
None. No auth, crypto, permission, or user-input handling is modified. The example snippets only show HTTP Basic Auth against the public API, which is already the documented integration pattern.
Level of scrutiny
Low. Docs/example fixes that correct field-name conventions ({aggregation}_{measure} instead of the previous {measure}_{aggregation}) and replace an unsupported top-level rowLimit with the supported nested config.row_limit. The risk is misleading copy-pasters, not breaking production code paths.
Other factors
I left three review comments on the prior revision (helper using endswith suffixes inconsistent with the new format, snake_case/camelCase split between orderBy and plot cell, and a stale rowLimit reference in the v2 launch changelog). Commit 769b4fc addresses all three: the helper now uses startswith with aggregation prefixes, the plot cell is consistently snake_case (time_dimension, p95_learning-style), and the changelog now references config.row_limit. A repo-wide grep confirms no remaining rowLimit references and no stale totalCost_sum / value_avg / latency_p95 / timeDimension references in v2 contexts — the surviving totalCost_sum hits in components/home/feature-tabs/data.ts are inside the legacy v1 snippet (view: \"traces\" + api.metrics.metrics) where {measure}_{aggregation} is the correct v1 format, and this file is untouched by this PR. Codex's earlier inline comment on data.ts referred to an earlier revision that has since been reverted out of the PR. Safe to approve.
What changed
orderBy.fieldexamples to use returned field names likesum_totalCost,avg_value, andtime_dimension.{aggregation}_{measure}.config.row_limitinstead of the unsupported top-levelrowLimitfield.Closes #2997
Validation
git diff --checktotalCost_sum,value_avg,"field": "timeDimension", androwLimitexamples.pnpm run format:checkwas not run successfully in the local sparse checkout because dependencies were not installed and localghauth was invalid, so the branch was published via the GitHub connector.Greptile Summary
This PR fixes incorrect field names and parameter keys in all Metrics API v2 examples. The
orderBy.fieldformat is corrected from{measure}_{aggregation}to{aggregation}_{measure}(e.g.totalCost_sum→sum_totalCost), and the unsupported top-levelrowLimitis replaced with the correct nestedconfig.row_limit.orderBy.fieldvalues tosum_totalCost,avg_value, andtime_dimensionacross the homepage snippet, docs page, cookbook MDX, and notebook.rowLimitwithconfig: {"row_limit": …}in every v2 example and updates the migration guide and prose accordingly.{aggregation}_{measure}naming convention.Confidence Score: 4/5
Safe to merge; all changes are documentation and code-example corrections with no runtime logic.
The four touched files are consistent and the fixes look correct. One stale
rowLimitreference remains in the 2025-12-17 changelog file, which was not included in this PR. If that entry was also wrong from day one it would continue to mislead developers who follow the changelog.content/changelog/2025-12-17-v2-metrics-and-observations-api.mdx still references
rowLimitat line 48 and may need a follow-up fix.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Client builds Metrics API v2 query] --> B{orderBy field?} B -->|aggregated metric| C["field: {aggregation}_{measure}\ne.g. sum_totalCost, avg_value"] B -->|time bucket| D["field: time_dimension"] A --> E{row limit?} E -->|override default 100| F["config: {row_limit: N}"] E -->|accept default| G["config omitted\n(default 100 rows)"] C --> H[POST /api/public/v2/metrics] D --> H F --> H G --> H H --> I[Response rows]Reviews (1): Last reviewed commit: "fix: correct Metrics API v2 examples" | Re-trigger Greptile