Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/scripts/check-telemetry-reads.sh
Original file line number Diff line number Diff line change
@@ -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" "$@"
81 changes: 81 additions & 0 deletions .github/scripts/check_telemetry_reads.py
Original file line number Diff line number Diff line change
@@ -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<reason>[^;]+);\s*"
r"expires (?P<date>\d{4}-\d{2}-\d{2});\s*approved-by (?P<owner>@?[\w.-]+)\s*$"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject punctuation-only approver placeholders

For any new direct read annotated with approved-by - or approved-by ..., this character class matches, and the exception is treated as valid based only on its reason and date. CI therefore suppresses the read even though no named owner exists to review it; require the approver value to contain at least one alphanumeric character before accepting the exception.

Useful? React with 👍 / 👎.

)


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: <reason>; expires YYYY-MM-DD; approved-by <name>"
)
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)
88 changes: 88 additions & 0 deletions .github/scripts/test_telemetry_reads.py
Original file line number Diff line number Diff line change
@@ -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()
20 changes: 20 additions & 0 deletions .github/workflows/repo-hygiene.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading