From c3c3be6809af2a6719a129e89cc7053a9244556a Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Sat, 17 Oct 2026 06:56:56 +0200 Subject: [PATCH 1/2] ci: stop new direct reads of cached telemetry outside the telemetry package A reading stays in telemetry.Store after its driver goes stale, offline or faulted. Every direct reader of RawW, SmoothedW, ReadingsByType or ReadingsByDriver must check driver health itself, and review caught the missing check in #36, #85, #310, #647, #664 and #740. The check counts those reads per file outside go/internal/telemetry and fails when a pull request adds one. Existing reads (147 today) may only go down. The error names the health-gated accessors to use instead. On the merged commits of #36, #85, #310 and #647 it fails. --- .github/scripts/check-telemetry-reads.sh | 42 ++++++++++++++++++++++++ .github/workflows/repo-hygiene.yml | 14 ++++++++ 2 files changed, 56 insertions(+) create mode 100755 .github/scripts/check-telemetry-reads.sh diff --git a/.github/scripts/check-telemetry-reads.sh b/.github/scripts/check-telemetry-reads.sh new file mode 100755 index 000000000..fa82e3d87 --- /dev/null +++ b/.github/scripts/check-telemetry-reads.sh @@ -0,0 +1,42 @@ +#!/usr/bin/env bash +# Fails when a change adds a direct read of cached telemetry outside +# go/internal/telemetry. A cached reading stays in the store after its +# driver goes stale, offline or faulted, so every direct reader has to +# remember to check health itself. Review found the forgotten check in +# #36, #85, #310, #647, #664 and #740. Existing reads are counted per file +# and may only go down. +# +# bash .github/scripts/check-telemetry-reads.sh [base-ref] +# +# An exception goes on the offending line: +# // telemetry-read-ok: ; expires YYYY-MM-DD; approved-by +set -euo pipefail +cd "$(git rev-parse --show-toplevel)" +BASE="${1:-${TELEMETRY_READS_BASE_SHA:-origin/master}}" +BASE="$(git merge-base "${BASE}" HEAD)" +PATTERN='\.(RawW|SmoothedW)\b|\.(ReadingsByType|ReadingsByDriver)\(' + +count() { + git grep -n -E "${PATTERN}" "$1" -- 'go/*.go' ':!go/internal/telemetry/' ':!*_test.go' 2>/dev/null \ + | grep -v 'telemetry-read-ok:' \ + | sed -E "s/^$1://" | cut -d: -f1 | sort | uniq -c | awk '{print $2, $1}' || true +} + +before="$(count "${BASE}")" +after="$(count HEAD)" +grew="$(join -a 2 -e 0 -o 0,1.2,2.2 <(echo "${before}" | sort) <(echo "${after}" | sort) | awk '$3 > $2')" + +if [ -n "${grew}" ]; then + echo "This change adds direct reads of cached telemetry:" >&2 + echo "${grew}" | awk '{printf " %s: %s -> %s\n", $1, $2, $3}' >&2 + cat >&2 <<'MSG' +A cached reading survives its driver going stale, offline or faulted. +Read live values through a health-gated accessor in go/internal/telemetry +(Store.SumOnlineEVW and Store.SumOnlineV2XW are the existing ones), or gate +each reading on Store.DriverHealth(r.Driver).TelemetryLive() and add the +accessor to go/internal/telemetry so the next caller cannot forget it. +MSG + exit 1 +fi +total="$(echo "${after}" | awk '{s += $2} END {print s + 0}')" +echo "telemetry reads outside go/internal/telemetry: ${total}, none added" diff --git a/.github/workflows/repo-hygiene.yml b/.github/workflows/repo-hygiene.yml index 21573cee1..06577bc81 100644 --- a/.github/workflows/repo-hygiene.yml +++ b/.github/workflows/repo-hygiene.yml @@ -25,3 +25,17 @@ 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 + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v7 + with: + fetch-depth: 0 + + - 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 From e6879c9c034a57f94306107a6bb5086dcc4180f6 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Sat, 17 Oct 2026 07:41:30 +0200 Subject: [PATCH 2/2] ci: count telemetry reads across platforms and validate exceptions Signed-off-by: Fredrik Ahlgren --- .github/scripts/check-telemetry-reads.sh | 43 +----------- .github/scripts/check_telemetry_reads.py | 81 ++++++++++++++++++++++ .github/scripts/test_telemetry_reads.py | 88 ++++++++++++++++++++++++ .github/workflows/repo-hygiene.yml | 8 ++- 4 files changed, 179 insertions(+), 41 deletions(-) create mode 100644 .github/scripts/check_telemetry_reads.py create mode 100644 .github/scripts/test_telemetry_reads.py diff --git a/.github/scripts/check-telemetry-reads.sh b/.github/scripts/check-telemetry-reads.sh index fa82e3d87..9e410bd8a 100755 --- a/.github/scripts/check-telemetry-reads.sh +++ b/.github/scripts/check-telemetry-reads.sh @@ -1,42 +1,5 @@ #!/usr/bin/env bash -# Fails when a change adds a direct read of cached telemetry outside -# go/internal/telemetry. A cached reading stays in the store after its -# driver goes stale, offline or faulted, so every direct reader has to -# remember to check health itself. Review found the forgotten check in -# #36, #85, #310, #647, #664 and #740. Existing reads are counted per file -# and may only go down. -# -# bash .github/scripts/check-telemetry-reads.sh [base-ref] -# -# An exception goes on the offending line: -# // telemetry-read-ok: ; expires YYYY-MM-DD; approved-by +# Counts cached telemetry reads per file, locally and in CI. +# Usage: check-telemetry-reads.sh [base-ref] set -euo pipefail -cd "$(git rev-parse --show-toplevel)" -BASE="${1:-${TELEMETRY_READS_BASE_SHA:-origin/master}}" -BASE="$(git merge-base "${BASE}" HEAD)" -PATTERN='\.(RawW|SmoothedW)\b|\.(ReadingsByType|ReadingsByDriver)\(' - -count() { - git grep -n -E "${PATTERN}" "$1" -- 'go/*.go' ':!go/internal/telemetry/' ':!*_test.go' 2>/dev/null \ - | grep -v 'telemetry-read-ok:' \ - | sed -E "s/^$1://" | cut -d: -f1 | sort | uniq -c | awk '{print $2, $1}' || true -} - -before="$(count "${BASE}")" -after="$(count HEAD)" -grew="$(join -a 2 -e 0 -o 0,1.2,2.2 <(echo "${before}" | sort) <(echo "${after}" | sort) | awk '$3 > $2')" - -if [ -n "${grew}" ]; then - echo "This change adds direct reads of cached telemetry:" >&2 - echo "${grew}" | awk '{printf " %s: %s -> %s\n", $1, $2, $3}' >&2 - cat >&2 <<'MSG' -A cached reading survives its driver going stale, offline or faulted. -Read live values through a health-gated accessor in go/internal/telemetry -(Store.SumOnlineEVW and Store.SumOnlineV2XW are the existing ones), or gate -each reading on Store.DriverHealth(r.Driver).TelemetryLive() and add the -accessor to go/internal/telemetry so the next caller cannot forget it. -MSG - exit 1 -fi -total="$(echo "${after}" | awk '{s += $2} END {print s + 0}')" -echo "telemetry reads outside go/internal/telemetry: ${total}, none added" +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 06577bc81..0b9111f30 100644 --- a/.github/workflows/repo-hygiene.yml +++ b/.github/workflows/repo-hygiene.yml @@ -28,13 +28,19 @@ jobs: telemetry-reads: name: no new direct telemetry reads - runs-on: ubuntu-latest + 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 }}