perf: publish properties with the nightly dataset and measure cached Trino JSON - #1224
Merged
Merged
Conversation
…Trino JSON
One nightly frozen-perf run produced two unrelated datasets: the table
suite published as posthog-file-views-v1 and the properties suite as
properties-sha256-<inventory hash>, so the dashboard's dataset selector
showed one or the other and no single view compared the engines across
both.
Publish both suites under the nightly's dataset version and tell them
apart with a suite column (tables | properties) on runs and
query_results. The properties inventory hash moves to a fixture_version
column so history still never mixes fixtures. The publisher's schema
bootstrap adds both columns and classifies existing rows by the
-properties run ID suffix; a summary without a suite publishes as
tables, and an unknown suite is rejected.
Also measure the properties JSON queries on trino_cached ("trino
(cache)"). It only ran the unsupported VARIANT representation, so cached
Trino had no properties coverage; now that the Hoglake connector honors
fs.cache.enabled, the measurement is meaningful. Update the README
paragraph that still said the connector ignores the cache.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Test Impact PlanDeterministic summary of how this PR changes tests, CI runners, and coverage-risk signals. Summary
Signals
Coverage risk: neutral or increased No coverage-reduction warnings detected. |
Review fixes: - Publish an explicit nightly_run_id (the table-suite run's ID) instead of leaving consumers to strip a -properties suffix from run IDs. - Make the column migration one-shot: a guarded DO block adds suite, fixture_version and nightly_run_id and classifies older rows only when suite is missing, instead of two UPDATE scans on every publish. - Add duckgres-perf-publisher --bootstrap-only and run it at the start of main scenario runs, so the schema (and a dashboard that depends on it) can land hours before the first data in the new shape; no window where the old dashboard picks the properties run as the latest run. - Move the suite constants to core and validate the suite when the step starts, so a typo fails before the benchmark instead of at publish. - Share the nightly dataset version through a YAML anchor. - README: correct the uncached-cache cutoff and stop claiming that fixture_version alone keeps history from spanning fixture changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both solved problems that did not justify permanent machinery. The bootstrap-only CLI mode and workflow step existed to close a one-time window of a few hours where the dev perf dashboard's table panels would be empty; merging the dashboard change right after the first publish does the same. The guarded DO-block migration avoided two UPDATE scans on a table that grows by ~200 rows a night; plain idempotent ALTERs and a WHERE suite IS NULL backfill are simpler and just as correct. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A frozen-perf nightly produces two suites, and they published as two unrelated datasets:
posthog-file-views-v1.properties-sha256-<inventory hash>.The dashboard's dataset selector could show one or the other. No single view compared Trino against Duckgres and Athena across the whole run.
This PR is the data half of a one-headline engine comparison. The dashboard half is a follow-up PR in PostHog/grafana-dashboards.
runsandquery_results:dataset_version, shared through a YAML anchor inposthog_frozen_perf.yaml. The properties step fails without it.suite(tables|properties) tells the suites apart. It's a closed set incore, validated when the step starts, so a typo fails before the benchmark rather than at publish.nightly_run_id(the table-suite run's ID) pairs a nightly's suites explicitly. A standalone run is its own nightly.fixture_versionrecords which properties fixture a run measured. Regenerating the fixture is a deliberate history break.perf-properties/directory, so the two result sets can't overwrite each other.ADD COLUMN IF NOT EXISTS) and backfills rows published before they existed, using the-propertiessuffix those rows carried (WHERE suite IS NULL, so rows with explicit values are never touched). Verified against PostgreSQL 17 with legacy rows and two consecutive bootstraps.tables.trino_cached, labelledtrino (cache). It previously ran only VARIANT, which Hoglake doesn't support, so every engine except cached Trino covered every intent. Now that the Hoglake connector honoursfs.cache.enabled(Support shared filesystem caching in Hoglake trino#43), the measurement means something.fs.cache.enabledis replaced; it has been wrong since that change.Rollout
The dashboard PR (PostHog/grafana-dashboards#539) reads the new columns, which the first publish after this merge creates. The current dashboard would pick the later-starting properties run as "the latest run" once both suites share a dataset.
posthog_frozen_perfon main.Between the publish and that merge, the current dashboard's table panels are empty (minutes, on a dev perf dashboard).
Testing
go test ./tests/perf/... ./tests/mw-dev/scenario/...passes, with new coverage for:Publisher: suite and fixture_version written on runs and results; the default suite; rejection of an unknown suite; the bootstrap ALTERs and backfills. Exec positions are now derived from the bootstrap length rather than hard-coded.
Runner: suite and fixture_version stamped on the summary.
Properties catalog: every Duckgres and Trino target measures JSON, and cached Trino JSON gets its own run label.
Frozen scenario:
properties_comparisonsharesperf_queries' dataset version.Scenario step: suite, fixture_version and nightly_run_id propagate to the summary; an unknown suite is rejected at step start.
Branch validation: run 35922189322 exercises the new cached-Trino properties path on mw-dev.
This is perf tooling only; no duckgres runtime change and no e2e harness change.
🤖 Generated with Claude Code