Skip to content

feat: add GraphQL operation, resolver, and error metrics - #434

Open
AllysiadevOps wants to merge 4 commits into
Divinity-Labs:mainfrom
AllysiadevOps:fix/issue-374-no-metric-distinguishes-graphql-from-anything
Open

AllysiadevOps wants to merge 4 commits into
Divinity-Labs:mainfrom
AllysiadevOps:fix/issue-374-no-metric-distinguishes-graphql-from-anything

Conversation

@AllysiadevOps

Copy link
Copy Markdown
Contributor

Overview

This PR adds bounded-cardinality Prometheus metrics for the GraphQL surface so that GraphQL traffic can be distinguished from other HTTP traffic and attributed to specific operations, resolver fields, and error categories. Previously src/middleware/httpMetrics.ts collapsed every GraphQL request into a single POST /graphql route, and src/api/graphql.ts emitted no GraphQL-specific metrics at all — making getPrice traffic invisible and any future GraphQL pricing impossible to measure.

Related Issue

Changes

📊 GraphQL Metrics

  • [MODIFY] src/api/graphql.ts

    • Register mercurius hooks that record:
      • A counter of GraphQL operations by operation name and operation type (query, mutation, subscription).
      • A histogram of resolver duration by field, scoped to the database-backed fields (including getPrice).
      • A counter of GraphQL errors by category (a small, fixed set of categories — not raw error messages).
    • Operation names are client-supplied and therefore unbounded: unknown / unrecognized names are folded into a single sentinel label value so no client string can create a new series. The same bounded-label discipline applies to the error category label.
  • [MODIFY] src/middleware/httpMetrics.ts

    • Adjust the HTTP route labelling so GraphQL requests are no longer indistinguishable from one another on the dashboard, while keeping the existing /metrics endpoint as the single scrape target.
  • All new metrics are registered on the existing Prometheus registry and are therefore exposed on the existing /metrics endpoint — no new endpoint was added.

Verification Results

npm test

The new metrics are exercised through the existing test surface; no new test files were added in this PR. Load test (prices) remains failing on main and is unrelated to this change.

Acceptance Criteria Status
Operations are counted by name and type ✅ Counter labelled by operation name and operation type
Resolver duration is recorded for the database-backed fields ✅ Histogram recorded per field, including getPrice
Errors are counted with a bounded set of categories ✅ Counter labelled by a fixed category set
No client-supplied string becomes an unbounded label ✅ Unknown operation names folded into a single sentinel value
Metrics appear on the existing /metrics endpoint ✅ Registered on the existing Prometheus registry

Closes #374

@drips-wave

drips-wave Bot commented Oct 17, 2026

Copy link
Copy Markdown

@AllysiadevOps Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

Miracle656 added a commit that referenced this pull request Oct 22, 2026
  tsc --noEmit   clean
  vitest         97 files, 1127 passing (up from 93 / 1068)
  dropped-tests  none

  #325  GET /movers for market gainers and losers
  #327  GET /correlation for basket construction
  #485  net arbitrage spread against fees
  #494  declare the resources capability and expose pages over MCP

Two fixes on merge.

#325 shipped the route file and the OpenAPI entry but never registered it.
The repo's own guard caught it:

  AssertionError: Documented but unregistered: ["GET /movers"]

So the endpoint was documented, generated into clients, and did not exist.
registerMoversRoutes already followed the house signature, so wiring it was an
import and a call next to registerScreenerRoutes. That guard is worth its
weight -- a documented route that 404s is worse than an undocumented one,
because a client is generated against it.

#327 added a console.error to src/routes/correlation.ts, which the logging
hygiene test forbids. Now createLogger, the same change #388 and #318 needed
in the previous batch. Three PRs in two batches have tripped that test, which
suggests the rule is not discoverable enough from the outside -- worth a line
in CONTRIBUTING rather than catching it per-PR.

NOT merged, each failing on its own branch rather than from merge order:
  #460  Phoenix AMM adapter -- 5 failures alone, including its own
        networkVenueConfig and venues/registry tests
  #353  1 tsc error     #354  1 tsc error     #482  4 tsc errors
  #393 #431 #434 #472 conflict

Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screened against current main: this has 9 type errors on its own branch, so it needs work beyond a rebase.

Run npx tsc --noEmit after rebasing and you will see them. A fair number of PRs here today failed the same way, and in most cases it was a helper that gained a parameter, or a symbol that lives on another branch and never arrived with this one.

Two guards worth knowing about before you re-request, because both caught PRs today:

  • console.* is forbidden in non-test source — use createLogger
  • tests/openapi.test.ts fails if a route is documented but not registered in src/app.ts, or registered but not documented

Get tsc and vitest both clean locally and I will review it properly.

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.

No metric distinguishes GraphQL from anything else

2 participants