[bug] PR Time to Merge should use average, not greatest

[jberkus]

When looking at larger intervals of time, Kubernetes PR-Time-To-Merge uses this calculation:

greatest(percentile_disc(0.5) within group (order by open_to_lgtm asc), 0) as m_o2l_a,

This is wrong; it leads to having a greater value the larger your time interval is, which is deceptive. For aggregating a median, there's two reasonable possibilities: a median of medians, or an average of medians. In practice, those two values are rarely that divergent, and an average is faster to calculate.

Therefore it should be:

avg(percentile_disc(0.5) within group (order by open_to_lgtm asc), 0) as m_o2l_a,

This mistake exists across all cacluations in this view.

Assigning to myself, will submit a PR later.

/assign

[lukaszgryglicki]

I'm OK with this change, just cc @caniszczyk - this is now calculated according to some specs that were given initially long time ago...

[caniszczyk]

+1 Cheers, Chris Aniszczyk https://aniszczyk.org

…

On Sat, Nov 16, 2024 at 8:41 AM Łukasz Gryglicki ***@***.***> wrote: I'm OK with this change, just cc @caniszczyk <https://github.com/caniszczyk> - this is now calculated according to some specs that were given initially long time ago... — Reply to this email directly, view it on GitHub <#85 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPSIKLL4NZM4UQ3ZSM6ML2A5KRPAVCNFSM6AAAAABR3XXOGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOBQGU4TSNRZGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[lukaszgryglicki]

Sp @jberkus pls create a PR and ping me, once merged I'll regenerate all dashboards - the question is:

• do we need this change in K8s only, or across all projects? If all then please also change metric in "metrics/all/xyz.sql" in addition to "metrics/kubernetes/xyz.sql".

[jberkus]

I haven't checked yet whether this is all projects.

[lukaszgryglicki]

I mean metric is the same in all projects, my question was: do you want to change in all projects or just in K8s?

[Arunodoy18]

Hi, i would love to contribute to this issue . .take-issue

[lukaszgryglicki]

Great, thank you.

[nomadicmehul]

Hi @lukaszgryglicki — I'd like to pick this up, but before writing any SQL I want to confirm scope, since I see #118 attempted this and was closed unmerged.

Two things I think are worth clarifying, because I believe the fix as written in the issue body can't be applied literally:

1. greatest(...) is not the aggregation.

The current code is:

greatest(percentile_disc(0.5) within group (order by open_to_lgtm asc), 0) as m_o2l_a

greatest(x, 0) here is a two-argument scalar clamp that floors negative durations at zero — it is not aggregating anything across sub-periods. So removing or replacing it does not by itself change how the value scales with interval width.

The proposed replacement in the description:

avg(percentile_disc(0.5) within group (order by open_to_lgtm asc), 0) as m_o2l_a

is not valid SQL avg() takes a single argument.

2. What the real change would be.

metrics/shared/metrics.yaml and metrics/kubernetes/metrics.yaml declare this metric as periods: d,w,m,q,y with aggregate: 1,7, and the SQL receives the window as {{from}}/{{to}}. So each period is computed independently over the raw PR set in that window there is no median-of-medians step anywhere today.

That means the actual complaint (a y value that reads high relative to the m values) is a methodology question: a single median over a year of a long-tailed distribution is not the same statistic as the average of that year's twelve monthly medians. Fixing it properly means bucketing inside the query and averaging the per-bucket medians, e.g.:

), bucketed as (
  select
    date_trunc('month', pr.created_at) as bucket,
    percentile_disc(0.5) within group (order by pr.open_to_lgtm asc) as med_o2l
  from tdiffs pr
  group by 1
)
select greatest(avg(med_o2l), 0) as m_o2l_a from bucketed