Skip to content

perf(mcp-analytics): compute tool latency quantiles in one pass - #122813

Open
pauldambra wants to merge 1 commit into
masterfrom
posthog/mcp-tool-tables-quantiles
Open

pauldambra wants to merge 1 commit into
masterfrom
posthog/mcp-tool-tables-quantiles

Conversation

@pauldambra

Copy link
Copy Markdown
Member

Problem

  • On the MCP tool detail page, the headline stats and the daily chart each compute p50 and p95 with two separate quantile calls.
  • ClickHouse keeps a separate sample for each call and does not combine them (docs), so p50 and p95 can come from different samples.
  • The daily chart has a hidden bug. A bucket with no durations returns NaN, and float(row[3] or 0) lets NaN through, because NaN is truthy in Python.
  • This follows a review finding on #122615, which made the same change to the tool quality query.

Changes

  • Both queries now use one quantiles(0.5, 0.95) call, so p50 and p95 come from one sample.
  • quantiles returns NaN, not NULL, for an empty input. An arrayMap maps NaN back to NULL. It runs once for each output row, not once for each event.
  • The daily chart shows 0 for a bucket with no durations, as it was meant to.
  • The stats query uses quantiles(...)(if(is_tool, duration_ms, NULL)), because HogQL accepts only one level for quantilesIf.
  • No visible change other than the NaN fix. The columns and the row mapping stay the same.

How did you test this code?

  • The new query shapes ran on real data in project 2. An all-NULL input gave NULL for each level.
  • Wall-clock runs of the old and new daily query over 30 days showed no measurable speed difference, because the event scan dominates.
  • Not run locally: the backend tests, because they need ClickHouse. CI runs them.
  • Not run locally: preflight and semgrep. The branch was built without a checkout. Ruff check and ruff format pass on both files.

Test rationale:

  • test_aggregates_scalars_and_intent_coverage now checks exact p50 and p95 values, not only "not None". This catches swapped array indexes.
  • The new test_returns_none_latency_when_durations_are_missing catches NaN getting through to p50_ms. No existing test covers a call with no duration.
  • test_groups_by_day_in_order now checks latency for a bucket with durations and a bucket without them. This catches the NaN bug in the daily query.

Release status

  • No feature flag controls this change

Docs update

None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5 (claude-opus-5-5)

  • Skills invoked: /writing-pr-descriptions.
  • The author asked for this as a separate PR after reviewing the same change on feat(mcp-analytics): add view toggles to tool quality charts #122615.
  • The duplicate search (gh pr list --search "mcp quantiles tool_tables") found no open PR.
  • No session material outside this repo is in the diff or this body.

Created with PostHog Desktop

Generated-By: PostHog Desktop
Task-Id: 5f7d1523-7072-4662-a685-6723b9b62f9b
@pauldambra pauldambra self-assigned this Oct 23, 2026
@trunk-io

trunk-io Bot commented Oct 23, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

⚠️ Duplication (Python) — 1 new duplicated block (worst 84 tokens)

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

First copy Second copy Lines Tokens
products/mcp_analytics/backend/hogql_queries/tool_quality_tables.py:385 products/mcp_analytics/backend/hogql_queries/tool_tables.py:529 16 84
✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

@pauldambra
pauldambra marked this pull request as ready for review October 23, 2026 18:50
@parameterai

parameterai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Risk: No findings

This PR replaces the separate p50/p95 quantile calls in the MCP tool stats and daily-stats HogQL queries with a single quantiles(0.5, 0.95) call, mapping NaN back to NULL so empty buckets render as 0 and missing durations yield None. All attacker-controlled inputs (tool name, interval, date range, property filters) remain bound as AST constants or via the unchanged _tool_call_where helper, and access validation is untouched, so the change does not alter any security boundary.

Sentinel reviewed 6956635 · Review settings

@pauldambra pauldambra added the stamphog Request AI approval (no full review) label Oct 23, 2026
@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team October 23, 2026 18:51

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved.

Contained read-only analytics change with targeted regression tests and no open substantive concerns. The trusted context reports a clean current-head review but does not specify its review state.

  • Author wrote 47% of the modified lines and has 6 merged PRs in these paths (familiarity MODERATE).
  • PostHog Review (Standard) reviewed the current head: no findings.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 72L, 1F substantive, 88L/2F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1b-small (88L, 2F, single-area, perf)
stamphog 2.6.1 .stamphog/policy.yml @ 6956635 · reviewed head 6956635

@trunk-io

trunk-io Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant