New Gate Criteria for Pull Requests

[jdmarshall]

I just wanted to make a formal statement about a decision that I only realized I'd made after acting on it several times, and plan to continue from here on.

TL;DR: Performance is now a feature and your Pull Requests will be judged by the regressions they introduce.

Long Version

As of almost a month ago, the Benchmark data is heavily informing the Yes/No decision on Pull Requests.

Telemetry libraries need to be fast, accurate, and cheap, not necessarily in that order. They are not playgrounds for what-ifs and feature factory work. If a PR introduces a performance regression, it had better be for reasons of correctness or security concerns.

The critical sections of the code are recording of data, such as Counter.inc(), and scraping, such as Counter.get() The former slows down the current requests, the latter introduces significant response time variability to all requests.

The Prometheus Client is particularly opinionated about constructors. Since we tend to make things at bootstrap time and reuse them at runtime, new features that make that phase of the workflow more complex don't really affect response times much. And in particular delegating some of that additional lifting to LabelMap is the preferred mechanism to reach feature parity between metric types.

Exceptions

In the interest of transparency, I will volunteer a loophole for this policy as stated.

I am a frequent and vocal defender of ergonomics and DX. If you can make a case that your feature removes a substantial footgun with the API, then I might be persuaded that the feature counts as 'Correctness'. Particularly if the regression is single digit percentage points.

For anything more than 5%, I'm probably going to want to gang a feature PR with a performance PR that zeros out some of the changes between versions. You will get a gold star for filing both yourself.