feat: refactor usage schema for richer event dimensions#5
feat: refactor usage schema for richer event dimensions#5lohanidamodar wants to merge 18 commits into
Conversation
Greptile SummaryThis PR replaces the freeform
Confidence Score: 4/5Safe to merge with one fix: The core refactor (schema, extractColumns, groupBy) is well-implemented and thoroughly tested. The one concrete defect is that src/Usage/Adapter/ClickHouse.php — specifically the Important Files Changed
Reviews (4): Last reviewed commit: "feat(database-adapter): validate groupBy..." | Re-trigger Greptile |
…apters - region column was sized at 2 (copied from country); region codes like 'us-east' and 'eu-central-1' are routinely longer. Bumped to 64. - Extracted the tag→column extraction, normalisation, and strict unknown-key check into Metric::extractColumns() so ClickHouse and Database adapters share one implementation.
…imit - hostname column 1024 -> 255. DNS hostnames are capped at 253 chars, so the wider size never gained anything but pushed the bloom_filter index past the InnoDB/utopia-php-database 768-byte single-attribute index prefix limit. - path column stays at 1024 for data fidelity (URLs with signed query strings exceed 255), but its index entry now uses an explicit lengths=[255] so MariaDB only indexes the first 255 chars. The index validator in utopia/database treats lengths[i] as the indexLength when set, taking the column under the 768 cap while preserving the full column for stored data. Caught by CI when the full Database adapter suite ran against MariaDB — local runs skipped 45 DatabaseTest cases because pdo_mysql wasn't loaded. Verified locally now via docker-compose: 195/195 passing.
Library-private extension that lives on UsageQuery (not the base utopia-php/query Query class) so cloud can express "group by service over 1d", "group by path x status over 1h" requests without forcing an SDK regeneration across every Appwrite client. - TYPE_GROUP_BY constant + static factory groupBy(attribute). - isMethod() accepts the new method so Query::parse() round-trips it from the JSON wire form. - isGroupBy / extractGroupBy / removeGroupBy helpers. extractGroupBy returns an array (multiple groupBy queries may coexist when bucketing by several dimensions at once), unlike extractGroupByInterval which is single-instance. - Unit tests cover the factory, isMethod, helpers, parse round-trip and the parsed-Query (base class) extraction path.
Extends findAggregatedFromTable so callers can bucket aggregated rows by arbitrary dimension columns (service, path, status, ...) alongside the existing time bucket. - parseQueries() collects groupBy attributes into parsed[groupBy] and validates each against the type-specific column set (Metric::EVENT_COLUMNS / Metric::GAUGE_COLUMNS) via the new validateGroupByAttribute() helper. Unknown columns raise a descriptive Exception listing what is allowed. - findFromTable() rejects groupBy without groupByInterval - keeps the contract simple (caller always supplies a time bucket too) and matches the Database adapter behaviour. - findAggregatedFromTable() appends the escaped dim columns to both SELECT and GROUP BY. parseAggregatedResults() already passes unknown keys through to the Metric so the dim values surface as plain getters (getService, getPath, ...). - Integration tests: single-dim groupBy(service) over 1d aggregates to the expected per-service sums; multi-dim groupBy(service)+groupBy(path) over 1h produces the cartesian grouping with correct sums per pair.
… ClickHouse The Database adapter does not push aggregation through utopia-php/database (no groupBy on the base Query class), so groupBy queries continue to be elided from the converted query list - callers get raw rows just as they do today with groupByInterval. The change is in validation: rejecting malformed groupBy use up front means the negative-case contract is identical on both backends. - validateGroupByQueries() runs at the top of convertQueriesToDatabase. Rejects groupBy attributes outside the union of EVENT_COLUMNS and GAUGE_COLUMNS (the Database adapter shares one collection for both types). Rejects groupBy without an accompanying groupByInterval. - Negative-case integration tests live in UsageBase so both adapters exercise them: unknown attribute throws, groupBy-without-interval throws.
groupBy support for aggregated findsAdds What landed
Tests
VerificationNotes / non-goals
|
Summary
userAgentraw column and thetagsJSON column on both tables.Test plan
vendor/bin/phpunitcomposer check