Skip to content

feat: add ordinal event dimension column - #40

Open
abnegate wants to merge 2 commits into
mainfrom
feat/event-ordinal-dimension
Open

abnegate wants to merge 2 commits into
mainfrom
feat/event-ordinal-dimension

Conversation

@abnegate

@abnegate abnegate commented Sep 4, 2026

Copy link
Copy Markdown
Member

Summary

  • Add ordinal to EVENT_COLUMNS, the event schema, and event indexes so additive counters (dedicated-database inbound / outbound / compute) can split per member the way gauges already do.
  • setup() ADD COLUMN IF NOT EXISTSs it onto existing event tables. The daily MV stays grouped on resource identity only, so a billing sum without an ordinal filter still totals every member.
  • ordinal becomes a shared dimension rather than a gauge-only one, so MetricTest::testOrdinalIsGaugeOnly is replaced by a case asserting it appears in both GAUGE_COLUMNS and EVENT_COLUMNS with an index on each.

Test plan

  • tests/Usage/MetricTest.php and ClickHouseColumnTypeTest.php (56 tests)
  • testEventOrdinalSeparatesReplicaSeries against ClickHouse (filter / groupBy / unsplit sum) — the Tests workflow runs the whole suite inside docker compose against a real clickhouse/clickhouse-server:25.11-alpine, and it is green on this head: OK (379 tests, 1736 assertions), run 35674840031
  • Rebased onto main: Metric.php, ClickHouse.php and MetricTest.php had taken five schema commits since this branch was cut (skip indexes, low-cardinality request dimensions, ipReputation), all absorbed by the merge in 302a204

Notes

Made with Cursor

Gauges already split multi-node resources on ordinal. Counters such as
dedicated-database inbound, outbound and compute need the same column so
a chart can tell members apart. setup() adds it to existing event tables.
The daily MV stays grouped on resource identity only, so billing still
sums every member.

Co-authored-by: Cursor <cursoragent@cursor.com>
@greptile-apps

greptile-apps Bot commented Sep 4, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The ordinal behavior appears correct, but the implementation-coupled tests violate an explicit repository requirement and must be replaced before merging.

Summary

The PR promotes replica ordinal from a gauge-only dimension to a shared event-and-gauge dimension, allowing event rows to be filtered and grouped per member while preserving unsplit daily billing totals.

  • Adds ordinal to event extraction, schema, indexing, and documentation.
  • Reuses the event schema for the Database adapter’s shared event/gauge collection.
  • Adds an observable ClickHouse integration test for filtering, grouping, and unsplit summation.
  • Also adds several implementation-coupled schema tests that violate the repository’s explicit testing rule.

Reviews (2) · Last reviewed commit: "chore: merge main into feat/event-ordina..."

Resolve Metric::getEventIndexes against the skip-index and prefixed-index
changes that landed on main, keeping ordinal in the set(0) index list.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 22, 2026

Copy link
Copy Markdown

Comments Outside Diff

These findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.

  • P2 Tests Mirror Internal Schema tests/Usage/MetricTest.php:677

    This test directly checks EVENT_COLUMNS, GAUGE_COLUMNS, generated schemas, and index IDs. Related tests also copy full column lists and assert the exact ClickHouse type chosen for ordinal. These checks mirror source configuration instead of testing observable behavior, contrary to the repository directive to test behavior and leave syntax and schema checks to linters. They are brittle under harmless schema refactoring and could still pass while an ordinal round trip is broken. Keep the event write, filter, group, and sum coverage, but remove these internal configuration assertions. This repository requirement must be satisfied before merging.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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.

1 participant