Skip to content

feat: add type and outcome event dimensions - #30

Open
lohanidamodar wants to merge 1 commit into
mainfrom
feat/type-outcome-dimensions
Open

feat: add type and outcome event dimensions#30
lohanidamodar wants to merge 1 commit into
mainfrom
feat/type-outcome-dimensions

Conversation

@lohanidamodar

Copy link
Copy Markdown
Contributor

Adds two metric-scoped event dimensions: type and outcome.

Why

Today a category has to be encoded into the metric name and parsed back out by the consumer:

  • auth.method.phone.{countryCode}~190 metric names for one concept. Billing can't group by it, so it enumerates every calling code into an IN list.
  • messages.{type}.{provider}[.sent|.failed] — a cross-product of nine names built from three numbers (recipients, delivered, failed), at three granularities.
  • webhooks.events.{sent|failed} — the same outcome split again.

With these two columns, the first becomes GROUP BY metric, type and the second collapses to two rows tagged type+outcome.

Metric-scoped by design

Both are read with metric pinned, so the same column carries different value spaces across metrics — a messaging channel for one, a calling code for another. That's the shape resourceId already has, where a bucket id and a function id share one column scoped by resourceType.

Deliberately not reusing status: it holds HTTP status, and sent/failed sort above 400 lexicographically, so an error filter (greaterThanEqual("status", "400")) would silently match every message row.

Details

  • set(0) indexed, like the other small closed value spaces (status, method, service, clientType). set supports ranges as well as equality, unlike bloom_filter.
  • LowCardinality(Nullable(String)) — each metric contributes a handful of values.
  • Sized type 64 / outcome 32.
  • Gauges unchanged. GAUGE_COLUMNS doesn't include them, so the gauge path still rejects both as unknown tags — verified.
  • setup() already emits ALTER TABLE … ADD COLUMN IF NOT EXISTS, so existing tables pick them up without a migration.

Verification

Confirmed end to end, since extractColumns() is strict (unknown tags throw rather than being dropped):

type     in EVENT_COLUMNS: yes
outcome  in EVENT_COLUMNS: yes
extractColumns: OK (type=sms outcome=sent)
gauge: correctly rejects type
index type       -> set(0)
index outcome    -> set(0)

And the query shapes the consumers need actually compile:

SELECT metric, SUM(value) as value, `type`
FROM `default`.`usage_events`
WHERE `metric` = {param_0:String} AND `time` >=AND `time` <GROUP BY metric, `type` ORDER BY value DESC

SELECT metric, SUM(value) as value, `type`, `outcome`
FROM `default`.`usage_events`
WHERE `metric` = {param_0:String}
GROUP BY metric, `type`, `outcome`

MetricTest::testEventColumnsConstant pins the column list and failed until updated — that guard did its job. Unit/schema/column-type tests green (56 tests, 288 assertions), Pint clean, PHPStan level max clean.

Not run: the ClickHouse e2e suite — no Docker daemon on this box. The DDL and query construction are verified above; actual execution against ClickHouse is CI's.

Two metric-scoped dimensions, so a metric can be broken down by what the
row is about and how the attempt ended without encoding either into the
metric name.

They replace the pattern where a category becomes part of the name and the
consumer parses it back out — auth.method.phone.{countryCode} is ~190
metric names for one concept, and messages.{type}.{provider}.{sent|failed}
is a cross-product of nine names built from three numbers.

Both are read with `metric` pinned, so the same column carries different
value spaces across metrics — a messaging channel for one, a calling code
for another. That is the shape resourceId already has, where a bucket id
and a function id share a column scoped by resourceType.

set(0) indexed like the other small closed value spaces, and
LowCardinality since each metric contributes only a handful of values.
Gauges are unchanged: GAUGE_COLUMNS does not include them, so the gauge
path still rejects both as unknown tags.
@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

Greptile Summary

Adds metric-scoped type and outcome dimensions for event measurements.

  • Extends the event column contract and schema with both dimensions.
  • Adds ClickHouse low-cardinality column handling and set indexes.
  • Updates the event-column contract test.

Confidence Score: 4/5

The PR is not safe to merge until the new type dimension is separated from the Database adapter's event/gauge discriminator.

Database writes allow the new tag-derived type value to overwrite the internal event discriminator, after which ordinary typed queries and totals no longer match the stored event.

Files Needing Attention: src/Usage/Metric.php and src/Usage/Adapter/Database.php

Important Files Changed

Filename Overview
src/Usage/Metric.php Adds the dimensions to the shared event contract, but type collides with the Database adapter's event/gauge discriminator and makes tagged events invisible to typed reads.
src/Usage/Adapter/ClickHouse.php Classifies both new event dimensions as nullable low-cardinality strings, consistently with their schema definitions.
tests/Usage/MetricTest.php Updates the constant-list assertion, but does not cover the cross-adapter type field collision.

Fix all with Greploop Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
### Issue 1
src/Usage/Metric.php:66
**Event type discriminator overwritten**

When a Database-backed event includes a metric-scoped `type` tag such as `type=sms`, extraction merges that value over the adapter's internal `type=event` discriminator, causing typed event queries and totals to omit the stored row.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "feat: add type and outcome event dimensi..." | Re-trigger Greptile

Comment thread src/Usage/Metric.php
@@ -64,6 +64,8 @@ class Metric extends ArrayObject
// sdk identity
'sdk', 'sdkVersion',
'deviceName', 'deviceBrand', 'deviceModel',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Event type discriminator overwritten

When a Database-backed event includes a metric-scoped type tag such as type=sms, extraction merges that value over the adapter's internal type=event discriminator, causing typed event queries and totals to omit the stored row.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Usage/Metric.php
Line: 66

Comment:
**Event type discriminator overwritten**

When a Database-backed event includes a metric-scoped `type` tag such as `type=sms`, extraction merges that value over the adapter's internal `type=event` discriminator, causing typed event queries and totals to omit the stored row.

**Knowledge Base Used:**
- [Usage domain model](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/usage/-/docs/usage-domain-model.md)
- [Usage storage adapters](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/usage/-/docs/usage-storage-adapters.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

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