-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add guidelines for metric graduation #8709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add guidelines for metric graduation #8709
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richabanker The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
| 1. **Testing requirement**: The metric must have comprehensive tests that validate: | ||
| - The metric is registered and emitted correctly | ||
| - The metric has the expected labels and values under known conditions | ||
| - The metric is included in the [stable metrics list](https://github.com/kubernetes/kubernetes/blob/master/test/instrumentation/testdata/stable-metrics-list.yaml). See the [instrumentation test README](https://github.com/kubernetes/kubernetes/tree/master/test/instrumentation/README.md) for steps on how to generate this file correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we can enforce with a presubmit? E.g. a hack verify script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is already a script for that in https://github.com/kubernetes/kubernetes/blob/master/hack/verify-generated-stable-metrics.sh, but I don't know if we updated that to work for Beta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script should only check for (BETA, STABLE) levels as those are checked by default here unless the --allstabilityclasses flag is passed, which we dont when generating the stable metrics, ref
So looks like we are covered here for BETA and STABLE. Though the name "verify-generated-stable-metrics.sh" is a little misleading since it doesnt contain beta. I'll see if I can improve that in a follow up.
|
|
||
| 2. **Stability validation**: The metric should have been at Beta stability for at least one release to ensure it has been sufficiently validated in production environments. | ||
|
|
||
| 3. **API Review**: Graduating a metric to Stable requires an API review by SIG Instrumentation, as it represents a contractual API agreement. See the [API Review](/contributors/devel/sig-instrumentation/metric-stability.md#api-review) section in the metrics stability documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ideally we want to be part of beta graduation? Most of the time by the time something is going stable it is too late to make changes without breaking existing users.
Or maybe we should do API review when it is added and for beta graduation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to only do it during the Beta graduation process, otherwise we will need to have a look at all the changes to Alpha metrics and that could take quite a toll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Damien said makes sense, I'll change to add the API review at the time of graduation to beta as well. We can keep reviewing for stable graduation too, and if/when turns out to be quite unactionable from our end - we can remove ourselves from stable graduation review process at that time ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking metric as stable is a commitment by SIG to maintain stability guarantees. I think it's more important for the owning SIG leads to review it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I added that for STABLE graduation, primarily a review from owning SIG is required along with an approval from SIG inst. Lmk if that sounds good or if we think that SIG Inst doesnt need to be present in the review process at all for graduation to STABLE.
contributors/devel/sig-instrumentation/metric-instrumentation.md
Outdated
Show resolved
Hide resolved
|
cc @rexagod |
| 1. **Testing requirement**: The metric must have comprehensive tests that validate: | ||
| - The metric is registered and emitted correctly | ||
| - The metric has the expected labels and values under known conditions | ||
| - The metric is included in the [stable metrics list](https://github.com/kubernetes/kubernetes/blob/master/test/instrumentation/testdata/stable-metrics-list.yaml). See the [instrumentation test README](https://github.com/kubernetes/kubernetes/tree/master/test/instrumentation/README.md) for steps on how to generate this file correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the case even for BETA metrics, otherwise our approval will not be required to validate the graduation.
| 1. **Testing requirement**: The metric must have comprehensive tests that validate: | ||
| - The metric is registered and emitted correctly | ||
| - The metric has the expected labels and values under known conditions | ||
| - The metric is included in the [stable metrics list](https://github.com/kubernetes/kubernetes/blob/master/test/instrumentation/testdata/stable-metrics-list.yaml). See the [instrumentation test README](https://github.com/kubernetes/kubernetes/tree/master/test/instrumentation/README.md) for steps on how to generate this file correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is already a script for that in https://github.com/kubernetes/kubernetes/blob/master/hack/verify-generated-stable-metrics.sh, but I don't know if we updated that to work for Beta
|
|
||
| 2. **Stability validation**: The metric should have been at Beta stability for at least one release to ensure it has been sufficiently validated in production environments. | ||
|
|
||
| 3. **API Review**: Graduating a metric to Stable requires an API review by SIG Instrumentation, as it represents a contractual API agreement. See the [API Review](/contributors/devel/sig-instrumentation/metric-stability.md#api-review) section in the metrics stability documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to only do it during the Beta graduation process, otherwise we will need to have a look at all the changes to Alpha metrics and that could take quite a toll
13b5bb0 to
e80ec21
Compare
e80ec21 to
9de615d
Compare
|
Ping for another look |
contributors/devel/sig-instrumentation/metric-instrumentation.md
Outdated
Show resolved
Hide resolved
d3fe14e to
a33d767
Compare
a33d767 to
0aa1588
Compare
|
|
||
| When graduating a metric from Alpha to Beta or from Beta to Stable, the following requirements must be met. For more information on stability levels and their guarantees, see [Metrics Stability](#metrics-stability). | ||
|
|
||
| #### Graduating to Beta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a section on cardinality requirements as well, that states that the owning SIG must review and propose, to the best of their ability, the "least cardinal" version of the metric being introduced that also fulfills their use-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure if we want to differentiate between ALPHA / BETA based on cardinality. Ideally thats a concern that should be thought through at the time when a new metric is being introduced (i.e. right from ALPHA). Plus "least cardinal" is kinda subjective too.. so not sure how to frame this clearly to avoid confusion.
contributors/devel/sig-instrumentation/metric-instrumentation.md
Outdated
Show resolved
Hide resolved
7b754e3 to
091e2db
Compare
|
ping @dgrisonnet |
091e2db to
4d97b9e
Compare
|
I think we should prioritize merging this since there are a bunch of metric graduation PRs open and likely there will be more in future. (sorry for the continuous pings!) |
870e12b to
1147118
Compare
|
|
||
| Wherever possible, ensure that metrics graduating to Beta follow the [Prometheus metric naming best practices](https://prometheus.io/docs/practices/naming/). | ||
|
|
||
| 1. **Testing requirement**: The metric must have a corresponding test that validates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw I slightly revised the testing requirements here. Ideally we would like for tests to validate that the metric behaves as expected when the relevant code instrumented with that metric is executed (instead of just verifying that the metric is registered with the right name / stability level / labels etc.)
Does this sound reasonable?
Co-authored-by: Marek Siarkowicz <[email protected]>
1147118 to
f4e5045
Compare
Which issue(s) this PR fixes: