Repository navigation
Expose event release lifecycle metadata - #688
alisawavezen12 wants to merge 3 commits into
Conversation
Include unchecked releases without events and ignore empty or placeholder release names.
neSpecc
left a comment
There was a problem hiding this comment.
A few notes on the release metadata. The most important one is the unguarded event.payload access in _composeEventWithRepetition; the rest are about consistency of release names and the cost/semantics of latestRelease.
| originalTimestamp: event.timestamp, | ||
| originalEventId: event._id, | ||
| timestamp: repetition.timestamp, | ||
| firstSeenInRelease: event.payload.release === undefined ? null : event.payload.release, |
There was a problem hiding this comment.
event.payload isn't guarded here. The daily events list explicitly tolerates events without a payload (sanitizeDailyEvent rebuilds it and logs "Missing event payload title"), but this line now throws a TypeError for such a row, and the whole dailyEventsPortion / repetitions response fails instead of showing one row with a fallback title. The new Event.firstSeenInRelease resolver already guards with payload && ....
Also, the same === undefined ? null : ... normalization is duplicated in the resolver, and the field is set only in the repetition branch (not in the no-repetition branch and not for the original appended in getEventRepetitions), so the factory returns different shapes depending on the path. Suggest setting it consistently in one place, e.g.:
firstSeenInRelease: event.payload?.release ?? null,| release: { | ||
| $type: 'string', | ||
| $regex: /\S/, | ||
| $not: /^\s*(null|undefined|nan)\s*$/i, |
There was a problem hiding this comment.
Placeholder and whitespace names ('', ' ', 'null', 'undefined', 'NaN') are filtered only here. firstSeenInRelease, resolvedInRelease and regressionInRelease are returned as is, and the release validator from codex-team/hawk.workers#590 treats these names as real releases when it orders them.
Example: CI sends release: process.env.VERSION while the variable is unset, so the newest release is stored as 'undefined'. latestRelease returns the previous real release X, so events first seen in the 'undefined' build aren't labeled New, while older events from X are. The validator can also set resolvedInRelease: 'undefined', which the API returns unchanged.
I'd normalize release names the same way on both sides (or not filter them here at all).
| const release = await this.collection.findOne({ | ||
| projectId: projectId.toString(), | ||
| release: { | ||
| $type: 'string', |
There was a problem hiding this comment.
$type: 'string' drops releases stored as numbers, while findByProjectAndRelease below explicitly falls back to numeric releases. For such a project latestRelease would be null (or a stale string release), while firstSeenInRelease of an event from build 42 is serialized by GraphQL as "42", so firstSeenInRelease === latestRelease never matches and the New label never shows up.
| * @param {ContextFactories} factories - model factories | ||
| * @returns {Promise<string|null>} | ||
| */ | ||
| latestRelease(project, _args, { factories }) { |
There was a problem hiding this comment.
This runs a separate findOne per project without a DataLoader, so a list like workspaces { projects { latestRelease } } turns into N queries.
The query itself can't use an index well either: the regex conditions on release don't bound the index scan, and the sort is by _id. So it either reads all of the project's releases and sorts them in memory, or walks _id backwards through the whole releases collection (worst for projects without releases). Consider a { projectId: 1, _id: -1 } index and/or a DataLoader keyed by project id.
| }, | ||
| }, { | ||
| projection: { release: 1 }, | ||
| sort: { _id: -1 }, |
There was a problem hiding this comment.
A question about semantics: "latest" here is the last created release document, i.e. when sourcemaps/commits were uploaded, not when the release was deployed or last seen in events. Cases where this gives the wrong answer:
- rollback: A and B are uploaded, then A is redeployed. A's document already exists, so latest stays B and new events in A are never New;
- one project with several release streams (e.g. web + mobile): latest is whichever was uploaded last, so the other stream's events are never New;
- a staging build uploaded before production becomes latest.
Would it be more reliable to also take into account the newest release seen in incoming events (repetitions store release since codex-team/hawk.workers#590)?
Adding "Resolved", "New", "Regression" label to events. Related codex-team/hawk.workers#590
Summary
firstSeenInRelease,resolvedInRelease, andregressionInReleasefields to GraphQL Event.