Skip to content

refactor(metrics): split the histogram runner calculation into named phases - #111229

Draft
posthog[bot] wants to merge 1 commit into
masterfrom
posthog-self-driving/refactormetrics-split-histogram-query-d3e3b5
Draft

posthog[bot] wants to merge 1 commit into
masterfrom
posthog-self-driving/refactormetrics-split-histogram-query-d3e3b5

Conversation

@posthog

@posthog posthog Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • Engineers who change the metrics heatmap must read one method that does five jobs: interval selection, query build and run, bounds validation, time grid construction, and count assembly.
  • MetricsHistogramQueryRunner._calculate scores C901 14, above the repository limit of 10.
  • Each new histogram behavior adds more branches to the same method.

Origin

  • Scout: a custom scout
  • First signal: 2026-10-03
  • Inbox report: open
  • Task started by: auto-start, after the report was rated P2 and ready to fix

Changes

  • _calculate now only orchestrates the phases. Each phase is a named private method: _resolve_interval, _build_runner, _execute_histogram_query, _shared_bounds, _grid_times, _grid_counts.
  • The change is mechanical. Users see no difference: the query, the error messages, the error order and the response stay the same.
  • The cell limit check stays in _calculate, between grid construction and count assembly, as before.
  • _calculate now scores below the C901 limit of 10.

How did you test this code?

  • Ran hogli test products/metrics/backend/tests/test_metrics_histogram_query_runner.py against the local dev stack. All 10 existing cases pass. They cover empty data, invalid ranges, metric type filters, grid boundaries, the cell limit, mixed bounds, weekly buckets and API scopes.
  • Ran ruff check, ruff format and a C901 check with a limit of 10 on the changed file.
  • Ran repo-wide uv run mypy --cache-fine-grained . with no issues.

Test rationale: No new test. The change preserves behavior, and the existing focused suite covers each extracted phase.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Automatic notifications

  • Publish to changelog?

Docs update

None. Internal refactor.

🤖 Agent context

Autonomy: Fully autonomous

Agent: PostHog Desktop (Claude Code), claude-opus-5-5

  • The work comes from a code-shape inbox report about the C901 score of this method.
  • Skills invoked: /writing-pr-descriptions.
  • No open PR covers this change.
  • One edge case stays the same on purpose: an explicit empty-string interval still falls back to the finest ladder step.

Created with PostHog Desktop from this inbox report.

🤖 Generated with Claude Code

…phases

Move interval selection, runner construction, query execution, bounds validation, time grid construction and count assembly out of MetricsHistogramQueryRunner._calculate into private methods. _calculate keeps the orchestration and the cell limit check. Query and response behavior do not change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 62e1a066-0f20-49eb-b6ca-62a29713feee
@posthog posthog Bot added the self-driving label Oct 3, 2026
@trunk-io

trunk-io Bot commented Oct 3, 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

@posthog posthog Bot assigned jzhu13 Oct 3, 2026
@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane (py:product:metrics)

This PR is assigned to the backend Python lane (py:product:metrics). It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

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.

✅ 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.

🚨 Comment density — 15% of added code lines are comments (9 of 59)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/metrics/backend/hogql_queries/metrics_histogram_query_runner.py 9 59

This check does not block merging. It updates on every push and clears when the share drops.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (7)
.agents/security.md — configured
.agents/skills/sending-notifications/SKILL.md — configured
docs/published/handbook/engineering/type-system.md — configured
docs/internal/person-data-access.md — configured
.agents/skills/adopting-generated-api-types/SKILL.md — configured
.claude/commands/conventions.md — configured
.agents/skills/writing-code-comments/SKILL.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
  • Review profile: QUIET
  • Plan: Enterprise
  • Run ID: 9ca7c727-f529-414b-9805-b6dca8e08098
📥 Commits

Reviewing files that changed from the base of the PR and between e7ba672 and b4b8590.

📒 Files selected for processing (1)
  • products/metrics/backend/hogql_queries/metrics_histogram_query_runner.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The histogram query runner now delegates interval selection, runner construction, histogram execution, shared-bound validation, grid-time generation, cell-limit enforcement, and count assembly to dedicated methods. _calculate assembles the response from these results.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to b4b85

No actionable issue is identified that would prevent merging after normal checks.

🚥 Pre-merge checks | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The Problem, Changes, testing, release status, notifications, and docs sections are present and specific. However, the agent context omits required definition-of-done details, including patch-coverage… Complete the agent context with the patch-coverage result or justification, a public-artifact statement, and the CodeRabbit CLI findings and disposition or the applicable reason it was skipped. State whether the new-events-schema gate appli…
Full details: Description check

Explanation

The Problem, Changes, testing, release status, notifications, and docs sections are present and specific. However, the agent context omits required definition-of-done details, including patch-coverage evidence, the public-artifact statement, and the CodeRabbit CLI result or reason for skipping it.

Resolution

Complete the agent context with the patch-coverage result or justification, a public-artifact statement, and the CodeRabbit CLI findings and disposition or the applicable reason it was skipped. State whether the new-events-schema gate applies and, if it does, provide its result.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@hosthog

hosthog Bot commented Oct 3, 2026

Copy link
Copy Markdown

HostHog preview — posthog-desktop-web

Latest build (b4b8590): https://c5f3f5488e704932b26189c8b730e6fc.hosthog.dev

Employee-gated; every push gets a fresh URL whose content never changes. All previews stop serving when the PR closes.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant