diff --git a/.github/scripts/check-telemetry-reads.sh b/.github/scripts/check-telemetry-reads.sh new file mode 100755 index 000000000..9e410bd8a --- /dev/null +++ b/.github/scripts/check-telemetry-reads.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash +# Counts cached telemetry reads per file, locally and in CI. +# Usage: check-telemetry-reads.sh [base-ref] +set -euo pipefail +exec python3 "$(dirname "$0")/check_telemetry_reads.py" "$@" diff --git a/.github/scripts/check_telemetry_reads.py b/.github/scripts/check_telemetry_reads.py new file mode 100644 index 000000000..8daef78a7 --- /dev/null +++ b/.github/scripts/check_telemetry_reads.py @@ -0,0 +1,81 @@ +"""Ratchet cached telemetry reads; exceptions need a reason, owner and expiry.""" +import collections +import datetime as dt +import os +import re +import subprocess +import sys + +# git uses the host's regex library. Keep this POSIX ERE portable to Apple Git. +GIT_PATTERN = r"\.(RawW|SmoothedW)([^[:alnum:]_]|$)|\.(ReadingsByType|ReadingsByDriver)[[:space:]]*\(" +READ = re.compile(r"\.(?:RawW|SmoothedW)\b|\.(?:ReadingsByType|ReadingsByDriver)\s*\(") +EXCEPTION = re.compile( + r"//\s*telemetry-read-ok:\s*(?P[^;]+);\s*" + r"expires (?P\d{4}-\d{2}-\d{2});\s*approved-by (?P@?[\w.-]+)\s*$" +) + + +def git(*args): + return subprocess.run(["git", *args], text=True, capture_output=True, check=True).stdout.strip() + + +def count(ref, today, validate): + result = subprocess.run( + ["git", "grep", "-n", "-E", GIT_PATTERN, ref, "--", "go/*.go", + ":!go/internal/telemetry/", ":!*_test.go"], text=True, capture_output=True, + ) + if result.returncode not in (0, 1): + raise RuntimeError(result.stderr.strip() or "git grep failed") + counts = collections.Counter() + for line in result.stdout.splitlines(): + _, path, number, source = line.split(":", 3) + matches = READ.findall(source) + if "telemetry-read-ok:" in source: + exception = EXCEPTION.search(source) + valid = False + if exception and exception["reason"].strip(): + try: + valid = dt.date.fromisoformat(exception["date"]) >= today + except ValueError: + pass + if valid: + continue + if validate: + raise ValueError( + f"{path}:{number}: invalid or expired telemetry exception; use " + "// telemetry-read-ok: ; expires YYYY-MM-DD; approved-by " + ) + counts[path] += len(matches) + return counts + + +def main(): + os.chdir(git("rev-parse", "--show-toplevel")) + base = sys.argv[1] if len(sys.argv) > 1 else os.environ.get("TELEMETRY_READS_BASE_SHA") or "origin/master" + base = git("merge-base", base, "HEAD") + today = dt.datetime.now(dt.timezone.utc).date() + before, after = count(base, today, False), count("HEAD", today, True) + grew = [path for path in sorted(after) if after[path] > before[path]] + if grew: + print("This change adds direct reads of cached telemetry:", file=sys.stderr) + for path in grew: + print(f" {path}: {before[path]} -> {after[path]}", file=sys.stderr) + print( + "A cached reading survives its driver going stale, offline or faulted.\n" + "Use a health-gated accessor in go/internal/telemetry (existing examples:\n" + "Store.SumOnlineEVW and Store.SumOnlineV2XW). Put the health check in the\n" + "accessor so each caller does not have to repeat it. Exceptions require\n" + "a reason, an expiry date and approved-by on the same line; the named\n" + "owner must review the exception in the PR.", file=sys.stderr, + ) + return 1 + print(f"telemetry read occurrences outside go/internal/telemetry: {sum(after.values())}, none added") + return 0 + + +if __name__ == "__main__": + try: + sys.exit(main()) + except (ValueError, RuntimeError, subprocess.CalledProcessError) as error: + print(str(error), file=sys.stderr) + sys.exit(1) diff --git a/.github/scripts/test_telemetry_reads.py b/.github/scripts/test_telemetry_reads.py new file mode 100644 index 000000000..aece6dcc4 --- /dev/null +++ b/.github/scripts/test_telemetry_reads.py @@ -0,0 +1,88 @@ +import datetime as dt +from pathlib import Path +import subprocess +import tempfile +import unittest + +SCRIPT = Path(__file__).with_name("check-telemetry-reads.sh") +HEADER = "package site\ntype Reading struct { RawW, SmoothedW float64 }\n" + + +class TelemetryReads(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.root = Path(self.temp.name) + self.git("init", "-q") + for key, value in [("user.name", "Fixture"), ("user.email", "fixture@example.invalid"), + ("core.hooksPath", "/dev/null"), ("commit.gpgsign", "false")]: + self.git("config", key, value) + self.write("go/site.go", HEADER + "func power(r Reading) float64 { return r.RawW }\n") + self.commit() + self.base = self.git("rev-parse", "HEAD").strip() + + def git(self, *args): + return subprocess.check_output(["git", *args], cwd=self.root, text=True) + + def write(self, path, text): + path = self.root / path + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + def commit(self): + self.git("add", ".") + self.git("commit", "-qm", "fixture") + + def check(self, expected, base=None): + result = subprocess.run(["bash", str(SCRIPT), base or self.base], cwd=self.root, text=True, capture_output=True) + self.assertEqual(result.returncode, expected, result.stdout + result.stderr) + return result.stdout + result.stderr + + def test_unchanged(self): + self.assertIn("1, none added", self.check(0)) + + def test_second_field_on_same_line(self): + self.write("go/site.go", HEADER + "func power(r Reading) float64 { return r.RawW + r.SmoothedW }\n") + self.commit() + self.assertIn("1 -> 2", self.check(1)) + + def test_second_call_on_same_line(self): + self.write("go/site.go", "package site\nfunc f(s Store) { s.ReadingsByType() }\n") + self.commit() + base = self.git("rev-parse", "HEAD").strip() + self.write("go/site.go", "package site\nfunc f(s Store) { s.ReadingsByType(s.ReadingsByDriver()) }\n") + self.commit() + self.assertIn("1 -> 2", self.check(1, base)) + + def test_new_file(self): + self.write("go/other.go", HEADER + "func extra(r Reading) float64 { return r.SmoothedW }\n") + self.commit() + self.assertIn("0 -> 1", self.check(1)) + + def test_exclusions(self): + for path in ["go/internal/telemetry/store.go", "go/site_test.go"]: + self.write(path, HEADER + "func extra(r Reading) float64 { return r.SmoothedW }\n") + self.commit() + self.check(0) + + def test_valid_exception(self): + expiry = dt.datetime.now(dt.timezone.utc).date() + dt.timedelta(days=30) + self.write("go/other.go", HEADER + f"func f(r Reading) float64 {{ return r.RawW }} // telemetry-read-ok: diagnostics; expires {expiry}; approved-by @frahlg\n") + self.commit() + self.check(0) + + def test_invalid_exceptions(self): + for suffix in ["expires 2000-01-01", "reason; expires 2000-01-01; approved-by owner", + "; expires 2999-01-01; approved-by owner", "reason; expires 2999-01-01", + "reason; expires 2999-02-30; approved-by owner"]: + with self.subTest(suffix=suffix): + self.write("go/other.go", HEADER + f"func f(r Reading) float64 {{ return r.RawW }} // telemetry-read-ok: {suffix}\n") + self.commit() + self.assertIn("invalid or expired", self.check(1)) + + def test_git_errors_fail(self): + self.check(1, "missing-ref") + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/repo-hygiene.yml b/.github/workflows/repo-hygiene.yml index 21573cee1..0b9111f30 100644 --- a/.github/workflows/repo-hygiene.yml +++ b/.github/workflows/repo-hygiene.yml @@ -25,3 +25,23 @@ jobs: env: PLANNING_DOCS_BASE_SHA: ${{ github.event.pull_request.base.sha }} run: .github/check-no-planning-docs.sh + + telemetry-reads: + name: no new direct telemetry reads + strategy: + matrix: + os: [ubuntu-latest, macos-latest] + runs-on: ${{ matrix.os }} + steps: + - name: Checkout + uses: actions/checkout@v7 + with: + fetch-depth: 0 + + - name: Test the telemetry check + run: python3 -m unittest discover -s .github/scripts -p 'test_telemetry_reads.py' + + - name: Count direct reads of cached telemetry + env: + TELEMETRY_READS_BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: bash .github/scripts/check-telemetry-reads.sh