Allow group of users to override commit queue benchmark failure

XMLWordPrintableJSON

    • Type: Task
    • Resolution: Done
    • Priority: Major - P3
    • 8.3.0-rc0
    • Affects Version/s: None
    • Component/s: None
    • 200
    • None
    • None
    • None
    • None
    • None
    • None
    • None

      Implementation sketch: Check PR for an appropriate comment that indicates to ignore this. We probably don't really need permissions here, since the most someone malicious could do is silently regress the benchmark, which seems unlikely.

      Currently, if someone's PR adds more than X number of CPU instructions (defined here) relative to its base commit the benchmarks_sep will fail in the commit queue and block the commit. In the majority of cases this is intended. However, there are some exceptions where we may be willing to accept a large increase instructions (e.g., for a correctness issue, etc.). In these scenarios we would like a specific group of people (let's say Product Perf, but this can be discussed) who have the permissions on GitHub to approve a PR/add it to the merge queue to allow a PR that is failing the thresholds check to merge. 

      Currently, the workaround for this is to submit a PR that increases the threshold, merge in whatever large change, and then submit another PR that decreases the threshold back down. This flow adds a lot of additional time (and complexity) for the engineer.

      Without this change we are blocked on tightening the thresholds for the SEP benchmark (which would allow us to catch regressions along the critical find path sooner) as we cannot do so without severely impacting developer productivity and velocity.

      Slack Thread captured from #sep-benchmark by john.daniels@mongodb.com

      • alice.doherty@mongodb.com: I'm currently updating the documentation for the commit queue benchmark and I'm curious what the desired workflow is for accepting a "larger than threshold" regression now that we're comparing instruction counts added to the base commit. It doesn't seem to make sense to increase the threshold (that seems only relevant if dealing with an increase in noise) but if there's a valid reason to accept a change that increases instructions over the threshold, how does that exception get merged?
      • alice.doherty@mongodb.com: BTW the PERF-7320)
      • daniel.hill@mongodb.com: for clang we increased it with a follow up PR to reduce it again. Unless we go by absolute numbers again I don't see a way around this
      • alice.doherty@mongodb.com: > for clang we increased it with a follow up PR to reduce it again
        sorry I keep forgetting this :joy: I guess the point I was trying to make is that the threshold right now is definitely on the lenient side and we have work to tighten it which is likely why we haven't had many ppl hit the threshold recently
      • alice.doherty@mongodb.com: > Unless we go by absolute numbers again I don't see a way around this
        I think there is a way to override commit queue checks - if someone has a good reason to add more than X instructions to the find path is that an option?
      • alice.doherty@mongodb.com: the other option is like what was done with clang - increase the threshold to something massive, merge the change in, and then reduce it back
      • amirsaman.memaripour@mongodb.com: It'd be nice to allow a group of users override the benchmark if needed – basically allowing a PR to get merged even if the benchmark is failing if those group approve the PR, or trigger the merge. Then server performance folks can work with the authors to decide if they PR should be exempted from the limits.
      • alice.doherty@mongodb.com: What team would be the one to talk to about this? I think that option is the cleanest
      • amirsaman.memaripour@mongodb.com: Initially, I thought a performance guild that includes performance-focused server engineers could own this, but then we have server performance which might be better positioned to make judgements on accepting/rejecting regressions.
      • alice.doherty@mongodb.com: Sorry to clarify, I mean what team is the one to talk to to enable the functionality to override a failing commit queue check (i.e., the GitHub commit queue/Evergreen tooling)
      • alice.doherty@mongodb.com: (If you know the answer, otherwise I can ask around)
      • amirsaman.memaripour@mongodb.com: Aa, sorry about the confusion. I imagine DEVPROD should be able to help us there?
      • alice.doherty@mongodb.com: john.daniels@mongodb.com is this something your team could help us with? We're looking to allow a certain group of people to approve a PR and allow it to merge even if it's failing the SEP instruction count check
      • john.daniels@mongodb.com: Yeah I think that makes sense. We still own the SEP benchmark logic. Can you start with a ticket?

            Assignee:
            Austin Hartschen
            Reporter:
            John Daniels
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated:
              Resolved: