From 40edf08600e3a37ec18baaf99a53af8107eb3c2d Mon Sep 17 00:00:00 2001 From: Julia Shtal Date: Tue, 15 Sep 2026 18:06:18 +0200 Subject: [PATCH] docs(metrics)!: restate the metric_snapshots comment and pin it to the calculators docs(metrics): point snapshot Javadoc at MetricSnapshotWriter docs(metrics): name MetricSnapshotWriter in knowledge-silo-score docs: correct twelve stale claims in CLAUDE.md chore: track docs/metrics by default docs: correct the secret defaults in README and complete .env.example --- .gitignore | 4 +- README.md | 9 +- dev-analytics/.env.example | 35 +++-- .../metrics/calc/MetricSnapshotWriter.java | 3 +- .../metrics/model/MetricSnapshot.java | 6 +- .../metrics/model/MetricType.java | 2 +- .../V66__metric_snapshots_comment_refresh.sql | 27 ++++ .../calc/AggregateStorageShapeDriftTest.java | 2 +- .../calc/MetricSnapshotTableCommentTest.java | 86 +++++++++++++ docs/metrics/knowledge-silo-score.md | 2 +- docs/metrics/timezone.md | 121 ++++++++++++++++++ 11 files changed, 277 insertions(+), 20 deletions(-) create mode 100644 dev-analytics/src/main/resources/db/migration/V66__metric_snapshots_comment_refresh.sql create mode 100644 dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/MetricSnapshotTableCommentTest.java create mode 100644 docs/metrics/timezone.md diff --git a/.gitignore b/.gitignore index ef2cd8e..c61db76 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,7 @@ docs/* +# Metric documents are thesis-canonical and belong in the deliverable. Specs, ADRs +# and internal notes stay local, which the docs/* rule above already handles. !docs/metrics/ -docs/metrics/* -!docs/metrics/author-attribution.md .claude/ .agents/ .idea diff --git a/README.md b/README.md index 544561a..d117a96 100644 --- a/README.md +++ b/README.md @@ -53,8 +53,8 @@ a React dashboard with AI-generated insights via local Ollama. | Variable | Purpose | Default | How to generate | |----------|---------|---------|----------------| -| `JWT_SECRET` | JWT signing key | dev key (**insecure**) | `openssl rand -hex 32` | -| `ENCRYPTION_KEY` | AES-256-GCM token encryption key (base64) | dev key (**insecure**) | `openssl rand -base64 32` | +| `JWT_SECRET` | JWT signing key | none — **startup fails** if unset | `openssl rand -hex 32` | +| `ENCRYPTION_KEY` | AES-256-GCM token encryption key (base64) | none — **startup fails** if unset | `openssl rand -base64 32` | | `POSTGRES_PASSWORD` | Database password | `123` (dev only) | choose one | | `SMTP_HOST` | SMTP server for password-reset emails | `mailhog` (Docker) / `smtp.gmail.com` (manual) | — | | `SMTP_PORT` | SMTP port | `1025` (Docker) / `587` (manual) | — | @@ -63,6 +63,11 @@ a React dashboard with AI-generated insights via local Ollama. | `OLLAMA_BASE_URL` | Ollama server URL | `http://localhost:11434` | — | | `COOKIE_SECURE` | Set `true` in production (HTTPS only) | `false` | — | +> `JWT_SECRET` and `ENCRYPTION_KEY` have no defaults. Unset, the application fails to +> start: `JWT_SECRET` shorter than 32 bytes raises `WeakKeyException`, and an empty +> `ENCRYPTION_KEY` raises `IllegalArgumentException: Empty key`. Both are thrown during +> bean creation, so the stack trace names the bean rather than the variable. +> > In production, always set `JWT_SECRET`, `ENCRYPTION_KEY`, and `POSTGRES_PASSWORD` > via environment variables or a secrets manager. Never commit real secrets. diff --git a/dev-analytics/.env.example b/dev-analytics/.env.example index 7e00aca..05ced3d 100644 --- a/dev-analytics/.env.example +++ b/dev-analytics/.env.example @@ -1,16 +1,35 @@ # Copy this file to .env and fill in the values before running docker compose up. -# Required — generate with: openssl rand -hex 32 +# -- Required ------------------------------------------------------------------ +# No defaults. The application fails to start if either is unset. +# Generate with: openssl rand -hex 32 JWT_SECRET= - -# Required — generate with: openssl rand -base64 32 +# 32 bytes, base64-encoded. Generate with: openssl rand -base64 32 ENCRYPTION_KEY= - -# Required — choose a strong password for the Postgres superuser +# Choose a strong password for the Postgres superuser POSTGRES_PASSWORD= -# Optional — needed only for password-reset emails -SMTP_HOST=smtp.gmail.com -SMTP_PORT=587 +# -- Database ------------------------------------------------------------------ +# Unset in Docker: docker-compose.yml points the app at the db service. Set these +# only when running the backend outside the compose network. +SPRING_DATASOURCE_URL= +SPRING_DATASOURCE_USERNAME= +SPRING_DATASOURCE_PASSWORD= + +# -- Email (SMTP) -------------------------------------------------------------- +# Optional - needed only for password-reset emails. Compose defaults to MailHog, +# viewable at http://localhost:8025. +SMTP_HOST= +SMTP_PORT= SMTP_USERNAME= SMTP_PASSWORD= + +# -- Ollama (AI) --------------------------------------------------------------- +OLLAMA_BASE_URL= +OLLAMA_MODEL= + +# -- App ----------------------------------------------------------------------- +APP_BASE_URL= +APP_FRONTEND_URL= +# Set true when serving over HTTPS +COOKIE_SECURE= diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/calc/MetricSnapshotWriter.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/calc/MetricSnapshotWriter.java index 7bf0dd3..b5111ee 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/calc/MetricSnapshotWriter.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/calc/MetricSnapshotWriter.java @@ -13,8 +13,7 @@ /** * Persists a {@link MetricSnapshot} using the native-SQL upsert guard. - * Extracted from {@code MetricsService.saveMetric} — logic is unchanged. - * All {@link MetricCalculator} beans call this service to write their results. + * Every {@link MetricCalculator} writes its results through this service. */ @Service @RequiredArgsConstructor diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/model/MetricSnapshot.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/model/MetricSnapshot.java index 1b91593..e3b1e75 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/model/MetricSnapshot.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/model/MetricSnapshot.java @@ -44,11 +44,11 @@ *

Invariant: {@code period_from IS NOT NULL AND period_to IS NOT NULL}.

* *

Upsert guard

- *

All writes go through {@code MetricsService.saveMetric}, which uses a native-SQL + *

All writes go through {@code MetricSnapshotWriter}, which uses a native-SQL * {@code findExisting} query with {@code IS NOT DISTINCT FROM} on nullable dimensions * ({@code repository_id}, {@code team_id}, {@code period_from}, {@code period_to}) to - * locate an existing row before inserting. Bypassing {@code saveMetric} will produce - * duplicate rows that aggregate incorrectly.

+ * locate an existing row before inserting. The table carries no unique key, so bypassing + * the writer produces duplicate rows that aggregate incorrectly.

* *

Scope

*

Personal metrics: {@link #team} is {@code NULL}. diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/model/MetricType.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/model/MetricType.java index 5b0ad95..aec19ee 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/model/MetricType.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/metrics/model/MetricType.java @@ -44,7 +44,7 @@ public enum MetricType { public final boolean inAiContext; /** True when the metric represents a daily count to be summed over the period (not averaged). */ public final boolean dailySum; - /** True when the metric is stored with periodFrom/periodTo instead of a date series. */ + /** True when the metric is recomputed per ISO week; storage shape is a calculator property. */ public final boolean aggregatePeriod; MetricType(boolean inAiContext, boolean dailySum, boolean aggregatePeriod) { diff --git a/dev-analytics/src/main/resources/db/migration/V66__metric_snapshots_comment_refresh.sql b/dev-analytics/src/main/resources/db/migration/V66__metric_snapshots_comment_refresh.sql new file mode 100644 index 0000000..51ac7ff --- /dev/null +++ b/dev-analytics/src/main/resources/db/migration/V66__metric_snapshots_comment_refresh.sql @@ -0,0 +1,27 @@ +-- V66: refresh the metric_snapshots table comment. +-- +-- V57 restated V40's comment and is itself stale on two counts: it names +-- MetricsService.saveMetric, which has been extracted to MetricSnapshotWriter, and its +-- AGGREGATE list omits REVIEW_PARTICIPATION_COUNT and WIP_OPEN_PR_AGE_HOURS_MEDIAN. +-- COMMENT ON TABLE has no partial form and V57 is frozen, so the comment is restated in full. +-- Comment-only — no DDL, no data change, and re-running it is a no-op. + +COMMENT ON TABLE metric_snapshots IS +'Persisted metric output. Two structurally distinct row shapes share this table: + DAILY (period_from IS NULL, period_to IS NULL): + date = calendar day measured; value = single-day figure. + Types: DAILY_COMMITS_COUNT, DAILY_COMMITS_AVG_SIZE, DAILY_PR_CREATED, + DAILY_PR_MERGED, DAILY_ISSUES_CREATED, DAILY_ISSUES_CLOSED, + DAILY_CHURN_RATIO, FOCUS_RATIO_DAYS_TASKS. + AGGREGATE (period_from IS NOT NULL, period_to IS NOT NULL): + date = snapshot capture date; period_from/period_to = calculation window. + Types: PR_LEAD_TIME_HOURS_MEDIAN, ISSUE_LEAD_TIME_HOURS_MEDIAN, + PR_FIRST_COMMIT_TO_MERGE_LEAD_TIME_HOURS_MEDIAN, + REVIEW_RESPONSE_TIME_HOURS_MEDIAN, REVIEW_PARTICIPATION_COUNT, + AFTER_HOURS_COMMIT_RATIO, DEEP_WORK_STREAK_DAYS, KNOWLEDGE_SILO_SCORE, + REFACTOR_RATIO, PR_SIZE_COMPLEXITY_SCORE, MERGE_WITHOUT_REVIEW_RATIO, + COMMITS_PER_WEEK_AVG, WIP_OPEN_PR_AGE_HOURS_MEDIAN. + The lists are exhaustive and disjoint: 8 DAILY + 13 AGGREGATE = 21 MetricType values. +Storage shape is a property of the calculator, not of MetricType.aggregatePeriod, which +selects ISO-week window resolution and is true for only five of the AGGREGATE types. +All writes go through MetricSnapshotWriter (upsert guard). team_id NULL = personal.'; diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/AggregateStorageShapeDriftTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/AggregateStorageShapeDriftTest.java index b03a65e..6dbb94b 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/AggregateStorageShapeDriftTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/AggregateStorageShapeDriftTest.java @@ -63,7 +63,7 @@ class AggregateStorageShapeDriftTest { * The metric types stored with {@code periodFrom}/{@code periodTo} — the contract the read * paths resolve against, and not derivable from {@code MetricType.aggregatePeriod}. */ - private static final Set EXPECTED_PERIOD_STORED = Set.of( + static final Set EXPECTED_PERIOD_STORED = Set.of( PR_LEAD_TIME_HOURS_MEDIAN, PR_FIRST_COMMIT_TO_MERGE_LEAD_TIME_HOURS_MEDIAN, ISSUE_LEAD_TIME_HOURS_MEDIAN, diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/MetricSnapshotTableCommentTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/MetricSnapshotTableCommentTest.java new file mode 100644 index 0000000..a2e8b19 --- /dev/null +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/MetricSnapshotTableCommentTest.java @@ -0,0 +1,86 @@ +package com.juliashtal.devanalytics.metrics.calc; + +import com.juliashtal.devanalytics.metrics.model.MetricType; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.jdbc.core.JdbcTemplate; + +import java.util.Arrays; +import java.util.EnumSet; +import java.util.Set; +import java.util.regex.Pattern; +import java.util.stream.Collectors; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Pins the {@code metric_snapshots} table comment against the calculators it describes. + * + *

The comment is the schema-level answer to "which shape is this metric stored in". Reading it + * back out of {@code pg_description} rather than restating it here is what makes an omission a + * build failure.

+ */ +@SpringBootTest +class MetricSnapshotTableCommentTest { + + @Autowired JdbcTemplate jdbc; + + private String comment; + + @BeforeEach + void readComment() { + comment = jdbc.queryForObject( + "SELECT obj_description('metric_snapshots'::regclass)", String.class); + assertThat(comment).as("metric_snapshots must carry a table comment").isNotBlank(); + } + + @Test + void tableComment_aggregateSection_namesExactlyTheTypesCalculatorsStoreWithAPeriod() { + assertThat(typesIn(sectionAfter("AGGREGATE"))) + .isEqualTo(AggregateStorageShapeDriftTest.EXPECTED_PERIOD_STORED); + } + + @Test + void tableComment_dailySection_namesExactlyTheTypesNotStoredWithAPeriod() { + Set expectedDaily = EnumSet.complementOf( + EnumSet.copyOf(AggregateStorageShapeDriftTest.EXPECTED_PERIOD_STORED)); + assertThat(typesIn(sectionBetween("DAILY", "AGGREGATE"))).isEqualTo(expectedDaily); + } + + /** An omission is only detectable by exhaustiveness: a missing type is silently absent. */ + @Test + void tableComment_bothSections_accountForEveryMetricType() { + Set documented = EnumSet.noneOf(MetricType.class); + documented.addAll(typesIn(sectionBetween("DAILY", "AGGREGATE"))); + documented.addAll(typesIn(sectionAfter("AGGREGATE"))); + assertThat(documented).containsExactlyInAnyOrder(MetricType.values()); + } + + /** Half of what V66 corrected was a class name; the type lists above do not cover it. */ + @Test + void tableComment_writerReference_namesTheClassThatActuallyWrites() { + assertThat(comment).contains(MetricSnapshotWriter.class.getSimpleName()); + } + + private Set typesIn(String text) { + return Arrays.stream(MetricType.values()) + .filter(t -> Pattern.compile("\\b" + t.name() + "\\b").matcher(text).find()) + .collect(Collectors.toCollection(() -> EnumSet.noneOf(MetricType.class))); + } + + private String sectionAfter(String marker) { + int i = comment.indexOf(marker); + assertThat(i).as("comment must contain a %s section", marker).isNotNegative(); + return comment.substring(i); + } + + private String sectionBetween(String start, String end) { + int i = comment.indexOf(start); + int j = comment.indexOf(end); + assertThat(i).as("comment must contain a %s section", start).isNotNegative(); + assertThat(j).as("comment must contain a %s section", end).isGreaterThan(i); + return comment.substring(i, j); + } +} diff --git a/docs/metrics/knowledge-silo-score.md b/docs/metrics/knowledge-silo-score.md index 2708d27..93044e5 100644 --- a/docs/metrics/knowledge-silo-score.md +++ b/docs/metrics/knowledge-silo-score.md @@ -35,7 +35,7 @@ knowledge_silo_score = MAX(share(R)) over all R with total_commits(R) > 0 - Attribution for numerator: `author_github_id = user.githubUserId` **OR** `lower(author_email) IN user.commitEmails`. The denominator counts every author and is not attributed. See [author-attribution.md](author-attribution.md). - Denominator: all commits regardless of author — no email filter. - Bot exclusion: `author_name NOT LIKE '%[bot]%'`, applied to both the numerator and the denominator. The score measures how concentrated *human* ownership of a repository is, so automated commits belong in neither term: counting them in the denominator alone would deflate every contributor's share in proportion to how much CI writes to the repo. See [author-attribution.md](author-attribution.md). -- Snapshots calculated before this policy took effect were computed against an unfiltered denominator and read lower. They are overwritten by the `MetricsService.saveMetric` upsert as each window is recalculated; no migration backfills them. +- Snapshots calculated before this policy took effect were computed against an unfiltered denominator and read lower. They are overwritten by the `MetricSnapshotWriter` upsert as each window is recalculated; no migration backfills them. - Saved as aggregate shape: `periodFrom = fromDate`, `periodTo = toDate`, `repository = null`. ## Edge cases diff --git a/docs/metrics/timezone.md b/docs/metrics/timezone.md new file mode 100644 index 0000000..eeaf5b7 --- /dev/null +++ b/docs/metrics/timezone.md @@ -0,0 +1,121 @@ +# Timezones and Day Boundaries + +**Applies to:** every metric in this directory +**Data source(s):** users.timezone, metric_snapshots.date, metric_coverage.date + +This page is the canonical definition of *which clock decides what "today" means*. Each metric +document states which rule it uses and refers here for the rule itself, so the definition exists +once rather than in nineteen partial copies. It is the time-side counterpart to +[author-attribution.md](author-attribution.md). + +## Principle + +A figure computed from the same input history must not change because the platform was deployed on +a machine in a different country. Wherever a calendar day has to be derived from an instant, the +zone is stated explicitly; nothing reads the JVM default. + +Two different zones are correct in two different places, and conflating them is the failure this +page exists to prevent. + +| Clock | Zone | Decides | +|---|---|---| +| **System clock** | always UTC | when a scheduled job fires, and which day it treats as "yesterday" | +| **Attribution clock** | the user's `users.timezone` | which of *that user's* calendar days an activity belongs to, and what counts as their working hours | + +The system clock answers "what day is it for the platform". The attribution clock answers "what day +was it for this developer". A developer in Auckland finishes a working day roughly thirteen hours +before a UTC-only reading agrees that it has finished; both statements are true of different +questions. + +## The system clock + +`SystemClock` wraps `Clock.system(ZoneOffset.UTC)` and is the only place a current instant enters +the application. Its no-argument `today()` and `yesterday()` read UTC. + +Every cron-scheduled job declares `zone = "UTC"`, so the firing time is a property of the +configuration rather than of the deployment: + +| Job | Schedule | +|---|---| +| `MetricsScheduler` | 01:00 UTC daily — computes yesterday for every user | +| `TokenCleanupScheduler` | 02:00 UTC daily — deletes expired refresh and reset tokens | +| `MetricBackfillScheduler` | 03:00 UTC daily — fills gaps in each user's collected history | +| `MetricsSummaryScheduler` | 08:00 UTC Mondays — generates the weekly AI summary | + +Jobs declared with `fixedRate` or `fixedDelay` (`CommitStatsEnrichmentScheduler`, +`SyncJobTracker`) carry no zone, because an interval has no wall-clock anchor to interpret. + +## The attribution clock + +`UserZone.of(user)` is the single reading of `users.timezone`. Two metrics and the backfill window +depend on it: + +| Site | Uses the user's zone for | +|---|---| +| `AFTER_HOURS_COMMIT_RATIO` | classifying each commit's wall-clock hour and weekday | +| `MetricBackfillService` | both ends of the window it fills — the earliest activity day, and the last day considered complete | +| `POST /api/metrics/backfill` | rejecting a requested `to` that is not yet a finished day for the requester | + +The backfill guard and the backfill window read the same zone deliberately: otherwise the endpoint +would refuse a day the nightly job already considered complete, or accept one it did not. + +### Resolving the zone + +`users.timezone` is `VARCHAR(64) NOT NULL DEFAULT 'Europe/Berlin'`. A user who never opens Settings +is therefore attributed in **Europe/Berlin**, not UTC — the column default is the product decision, +and UTC is only a defensive fallback: + +| Stored value | Resolves to | +|---|---| +| a valid IANA zone id | that zone | +| unparseable | `ZoneOffset.UTC`, with a warning logged against the user id | +| null or blank | `ZoneOffset.UTC` — unreachable through the database, which rejects null | + +DST is handled by `ZoneId` itself: each instant is offset by the rule in force at that instant, so +a window spanning a transition is not skewed. + +### Changing a timezone does not recompute anything + +Unlike changing an identity, editing `users.timezone` does not invalidate stored snapshots. Rows +already written under the previous zone survive, so an `AFTER_HOURS_COMMIT_RATIO` series can +contain days classified under two different zones. The value is recomputed only when the day is +recalculated for another reason. + +This is a deliberate asymmetry: an identity change alters *which records belong to the user*, which +makes every stored figure wrong; a zone change alters only the interpretation of a boundary. + +## Storage + +`Instant` is used for stored timestamps and at API boundaries; the columns are `timestamptz` +(migrations `V46`, `V55`), so Postgres preserves the offset and comparisons are unambiguous. +`LocalDate` is used only for calendar-day windows — `metric_snapshots.date`, `period_from`, +`period_to` — where the day has already been decided by one of the two clocks above. `LocalDateTime` +is not used anywhere: it is an instant with the zone silently removed. + +## Known divergence: daily bucketing follows the database session zone + +The daily metrics bucket rows with `date()` in the repository queries. On +PostgreSQL that cast resolves in the **session** `TimeZone`, which the JDBC driver sets from the +JVM default; `hibernate.jdbc.time_zone` is not configured. The day a commit is assigned to +therefore follows the server's zone, not UTC. + +Observed directly against the project database for the instant `2026-03-14 23:30:00+00`: + +| JVM / session zone | `date(...)` returns | +|---|---| +| `UTC` | `2026-03-14` | +| `America/Los_Angeles` | `2026-03-14` | +| `Pacific/Auckland` | `2026-03-15` | + +The windows passed into those queries *are* UTC-explicit (`MetricsService` converts `from`/`to` with +`atStartOfDay(ZoneOffset.UTC)`), so only the bucketing inside the window is affected. The specified +rule for every daily metric remains UTC day boundaries, as each metric document states; the +implementation does not yet meet it, and the affected metrics are: + +`DAILY_COMMITS_COUNT`, `DAILY_COMMITS_AVG_SIZE`, `DAILY_CHURN_RATIO`, `DAILY_PR_CREATED`, +`DAILY_PR_MERGED`, `DAILY_ISSUES_CREATED`, `DAILY_ISSUES_CLOSED`, `DEEP_WORK_STREAK_DAYS`, +`FOCUS_RATIO_DAYS_TASKS`. + +Two installations in different zones will disagree about which day a commit made near midnight +belongs to. Within one installation the bucketing is self-consistent, so a single deployment's +series is internally comparable.