fix: filter maintainer counts to active maintainers only (CM-1466) - #4676
joanagmaia wants to merge 4 commits into
Conversation
Signed-off-by: Joana Maia <jmaia@contractor.linuxfoundation.org>
PR SummaryMedium Risk Overview
Reviewed by Cursor Bugbot for commit 161606e. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
🟢 Approval recommended
The filter matches the established active-role semantics and datasource types.
Pull request overview
Filters member roles to active assignments, correcting maintainer tags and counts.
Changes:
- Excludes future and end-dated maintainer roles.
- Preserves the
1970-01-01active-role sentinel.
File summaries
| File | Description |
|---|---|
services/libs/tinybird/pipes/member_roles.pipe |
Adds active-date filtering to member roles. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… pipe (CM-1466) Signed-off-by: Joana Maia <jmaia@contractor.linuxfoundation.org>
8b2ed2d to
63b5094
Compare
gaspergrom
left a comment
There was a problem hiding this comment.
Solid fix for the two range bugs described. One question on the temporal definition before this merges.
| {% end %} | ||
| ) | ||
| AND startDate <= now() | ||
| AND (endDate = toDateTime64('1970-01-01 00:00:00', 3) OR endDate >= now()) |
There was a problem hiding this comment.
Curious about the cutoff choice here. Two other pipes handle maintainer end dates with a grace period. health_score_v2_maintainer.pipe keeps someone counted for 12 months past endDate when paired with a recent-activity check, and org_page_kpis_copy_pipe.pipe uses a 730-day grace window. A role that ends today drops out of this count today, no grace window. Was a hard cutoff a deliberate choice for CM-1466, or should this line up with the grace-period pattern used elsewhere?
There was a problem hiding this comment.
Worth explaining the reasoning instead of leaving it as an unstated cutoff:
The core issue is that endDate on a maintainer role isn't a reliable signal of when someone stopped being a maintainer, it's set whenever our processing stops detecting that role in the source (e.g. we can no longer find the identity, or the maintainer file changed in a way we don't parse). So endDate tells us "we no longer consider this person an active maintainer as of this date," not "this person actually stopped maintaining on this date." Given that, adding a grace period on top of endDate doesn't buy us anything — we'd just be extending an already-unreliable timestamp by an arbitrary amount. The intent of this fix is to always count currently active maintainers, full stop, not tie it to any window.
On org_page_kpis_copy_pipe.pipe / org_page_projects_copy_pipe.pipe — I'm intentionally leaving those alone. They back org pages that other teams consume, and those pages support a time-range display we don't have on Insights. Changing their windowing here would affect output outside this PR's scope.
health_score_v2_maintainer.pipe's 12-month window is a different case, not the same inconsistency: bus factor there isn't just reading the maintainer role data — it's driven by observed contributor activity (review/merge actions in activityRelations), with the curated role data only used as a secondary signal capped by that same recent-activity requirement. It's answering "who's actually still active on this repo," not "who currently holds the maintainer role," so it doesn't need to match this fix.
Let me know if this is clear for you
There was a problem hiding this comment.
Update PR description as well
gaspergrom
left a comment
There was a problem hiding this comment.
Question's been addressed, looks good now.
Summary
member_roles.pipehad no active-maintainer filter (WHERE 1 = 1), so it counted every maintainer role assignment ever recorded, including ones end-dated years ago. Verified against LFX standard metrics and CDP's own Postgres across 18+ CNCF projects post-fix — exact match on distinct-member counts (e.g. cilium 1914 tagged vs 51 correct; opentelemetry 344 vs 19 correct).active_contributors.pipe'smaintainers_countnode already had a sentinel filter, but used range-overlap semantics tied to the caller'sstartDate/endDate(endDate >= startDateinstead ofendDate >= now()). Since the "Active Contributors" widget always calls it with a rolling window (e.g. last 365 days), this counted anyone whose maintainer role was active at any point in that window, including people whose role has since ended — reproducing the same inflated number (cilium: 51 with no date range, 1914 with a 365-day range). This is the pipe that actually feeds the widget's "Maintainers" figure, notmember_roles.pipe.Both fixes apply the same point-in-time active-maintainer definition now used by
member_roles.pipe(this PR): a role is active ifstartDate <= now()andendDateis either the "still active" sentinel (1970-01-01) or in the future, decoupled from any caller-supplied date range.Why "active now", not a grace window
endDateon a maintainer role isn't a reliable signal of when someone actually stopped maintaining — it's set whenever our processing stops detecting that role in the source (identity no longer found, maintainer file format changed, etc.). So it means "we no longer consider this person an active maintainer as of this date," not "this person's maintainership ended on this date." Given that, layering a grace period on top ofendDatedoesn't add signal — it just extends an already-unreliable timestamp by an arbitrary amount. The goal here is to always count currently active maintainers, not maintainers active within some window.Scope: why other maintainer-reading pipes are untouched
org_page_kpis_copy_pipe.pipeandorg_page_projects_copy_pipe.pipekeep their 730-day grace window on purpose. They back org pages that other teams already consume, and those pages support a time-range display concept we don't have on Insights — changing their windowing here would shift output outside this PR's scope. That's its own ticket, with those teams looped in, not something to bundle into this fix.org_dash_maintainers.pipeis also untouched — it's a time-series chart (bucket-overlap semantics), not a point-in-time count, so this fix doesn't apply to it.health_score_v2_maintainer.pipeandhealth_score_v2_raw_inputs_snapshot.pipeare a different case entirely, not the same inconsistency: bus factor there isn't reading the maintainer role data as its primary signal — it's driven by observed contributor activity (review/merge actions inactivityRelations), with the curated maintainer role data only used as a secondary signal, itself capped by a recent-activity requirement. It's answering "who's actually still active on this repo," not "who currently holds the maintainer role," so it doesn't need to match this fix.health_score_v2_raw_inputs_snapshot.pipemirrors this definition on purpose, per its own description, to reconstruct the same populationhealth_score_v2_maintainerscores from for validation — so it stays in lockstep with that pipe, not withmember_roles/active_contributors.Related to CM-1466 (CNCF maintainer detection scoping) — this closes the outstanding Tinybird gaps in the same maintainer-data-quality effort, scoped to the two pipes actually affected by the reported bug.