Generate unique check IDs and detail routes#16
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
maquchizi
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12b82f623f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * leads so URLs stay scannable and sort/autocomplete by service name. | ||
| */ | ||
| export function checkId(job: string, instance: string): string { | ||
| return `${baseCheckId(job, instance)}-${shortHash(checkIdentity(job, instance))}`; |
There was a problem hiding this comment.
When an installation has many checks sharing the same job slug (for example one job name monitoring many targets) or a crafted pair of targets, this 32-bit FNV suffix can collide, producing duplicate /site/<id> links and React keys; getSiteHistory() then resolves the first matching check, leaving the other target without an addressable detail page. Use a longer/collision-resistant digest or include an encoded target component so the public ID is actually unique for the (job, target) pair.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
While the chance of a collision at 32-bit is "highly unlikely", moving to 64 bit makes it "practically impossible" at the cost of a longer (uglier?) hash. I think that's a fair trade off so I've made the change.
25cc850 to
e64d245
Compare
* We use (job, target) to generate a unique, deterministic ID for each
check that we use in the URL and as an ID in the frontend
* I considered only appending the ID to the URL in cases where there
were collisions but that ended up being more unstable and generating
edge cases like: What happens when there was no collision then
someone adds a check that collides with another? To which check do
we append the ID? What happens when the collision goes away? Do we
drop the ID? This is not good for deeplinking and I went with
stability and functionality over beautiful URLs here.
* Also included a migration note as this change will break previously
existing URLs
* Because we need to use Bigint for the 64 bit hash
e64d245 to
e029b5a
Compare
kilemensi
left a comment
kilemensi
left a comment
There was a problem hiding this comment.
LGTM!
Not sure if these URLs should show up as page title though... I assumed the title would be some combination of name, group, org for the URL.
* Use the job and target to build the page title. Dedup them if they're the same * Fallback to raw ID if construction fails for any reason * Also added some unit tests to validate this behaviour
|
@kilemensi That's what happens when you test on a browser with a thousand tabs open, you never get to see page titles. Good catch, I've fixed that. |
2 participants
What & why
Public check IDs were derived from
slugify(job)alone, but Grafana defines a check's identity as job name + target (documented here: "all checks include theJob nameandTargetoptions, which together uniquely identify the check"). As a result, two checks sharing a job name with different targets - or two job names that normalize to the same slug (e.g.Public APIandPublic-API) - collided onto one ID. That single ID was then used as the React list key and the/site/<id>link on the overview, and as the lookup key ingetSiteHistory()(which returns the first match), so colliding checks got duplicate keys/links and at least one had no independently addressable detail page.This change gives every check a unique, deterministic ID:
`<job-slug>-<hash>`, where the hash is a stable digest (FNV-1a) of the full(job, target)identity. Because the hash depends only on the check's own identity, an ID never changes when some other check is added, removed, or renamed - it only changes if that check's own job or target changes. The readable slug leads so/site/<id>URLs stay scannable and sort/autocomplete by service name.Closes #10
Type of change
Checklist
AGENTS.mdandCONTRIBUTING.mdpnpm checkpassespnpm testpassespnpm test:typespassespnpm buildsucceedsdocs/) if behaviour or configuration changedNEXT_PUBLIC_prefixNotes for reviewers
Breaking: all
/site/<id>URLs change. Every ID gains a hash suffix (pesacheck-org→pesacheck-org-1a2b3c), so existing bookmarks/deep links will 404 and must be regenerated. This was a deliberate trade - always-appending gives per-check ID stability that never depends on sibling checks, at the cost of URL compatibility. The alternative (append the hash only on collision, keep bare slugs otherwise) preserves non-colliding URLs but lets a check's URL flip the moment a colliding sibling appears. Migration note is documented indocs/architecture.md. In production the change lands after the ISRREVALIDATE_SECONDSwindow (or a redeploy); in local dev you may need to clear.next/cachesince the pre-change overview is cached.Where to look:
lib/format.ts- newcheckId(job, target)(pure, always-append) plus sharedcheckIdentity/fnv1ahelpers.lib/synthetics.ts-listChecks()assignscheckIdper deduped(job, instance); a duplicated localkey()helper was removed in favour ofcheckIdentity.lib/mock.ts- derives IDs via the samecheckId; addsPublic API/Public-APIfixtures that collide after slug normalization, so the mock (and therefore the credential-free e2e run) exercises disambiguation end to end. The fixture RNG is now seeded on the stable(job, target)identity rather than the public ID.Testing: unit tests cover same-job/different-target and slug-normalization collisions (
lib/format.test.ts,lib/synthetics.test.ts,lib/mock.test.ts); e2e asserts colliding rows get distinct, working links (e2e/home.spec.ts) and resolves detail URLs from the overview rather than hardcoding slugs (e2e/site.spec.ts). Verified against live Grafana data: 49 services, 49 unique IDs, zero duplicate links.