-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
add deprecation policy for k8s metrics #27202
Conversation
/assign @lavalamp |
/sig instrumentation |
Deploy preview for kubernetes-io-master-staging ready! Built with commit b95b750 https://deploy-preview-27202--kubernetes-io-master-staging.netlify.app |
|
||
* **STABLE metric: 3 releases** | ||
* **STABLE (but deprecated): 2 releases** | ||
* **STABLE (but deprecated and now hidden by default): 1 release** |
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 list isn't super clear, are these separate steps? Or do they overlap?
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.
ah, they overlap. I'll rephrase.
* **STABLE (but deprecated and now hidden by default): 1 release** | ||
|
||
Deprecated metrics have the same stability guarantees of their counterparts. If a stable | ||
metric is deprecated, then a deprecated stable metric is guaranteed to not change. When |
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 sentence is super confusing but I don't know how to help it. Maybe don't make "alpha and deprecated" to be a possible thing. Alpha metrics just get removed, not deprecated. So all "deprecated" metrics must have once been stable, and therefore can't be changed, just like other stable metrics.
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.
sgtm
which the metric will be considered deprecated. | ||
|
||
Deprecated metrics will have their description text prefixed with a deprecation notice | ||
string '(Deprecated from x.y)' and a warning log will be emitted during metric |
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 is the point to this warning log? consumers of the metrics aren't going to be looking at the logs 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.
I disagree actually. Cluster admins also tend to also be consumers of metrics, so they may end up looking at the logs.
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.
Sure, I don't object to logging them, but I can't believe it's going to happen very often that the person in charge of metrics ingestion is going to happen to be reading the logs and learn about the deprecation there for the first time, in advance of their metrics ingestion breaking.
2 releases is pretty short for this. 4 would align more with api deprecation. I think you should get someone other than me from sig arch to sign off too. Maybe @liggitt. |
It's actually 3 releases. But yeah we can modify it to be 4. |
I guess it depends where you are counting from though. |
/assign @liggitt |
Yeah I was counting until they disappear by default. |
Is this a change for v1.21 or for v1.20 @logicalhan ? (If it's for v1.21, the PR should target dev-1.21 - master represents the live website). |
Updated :) |
/milestone 1.21 |
/assign |
/assign @PI-Victor |
It is indeed. |
|
||
**Rule #10: STABLE metrics must undergo a deprecation lifecycle prior to removal.** | ||
|
||
* **STABLE metric: 3 releases** |
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.
Suggest a X releases or X months, whichever is longer
approach (matching GA API lifetimes would make sense to me)
Also, the 12 month guaranteed lifetime for GA APIs looks increasingly insufficient. We have enormous difficulty getting people to move from beta APIs to GA APIs in a year, let alone moving from one GA API to another. I think the promise of GA APIs (or "stable" metrics in this case) is effectively forever
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.
We have a built-in forcing function for migration. We break people but allow them to unbreak themselves for a release to migrate.
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.
We break people but allow them to unbreak themselves for a release to migrate.
That doesn't match my understanding of "stable" or "GA".
@kubernetes/sig-instrumentation-approvers , please provide a technical review for this PR by March 31 to get this into the release. Thank you! |
Hi @brancz @x13n @DirectXMan12 @lavalamp @dashpole @ehashman @mml @sttts @bsalamat @andrewsykim you are listed as reviewers for KEP-1209: Metrics Stability Framework i would like to ask you to please provide a tech review of this PR by March 31st in order for it to make it into the release. |
Hi @logicalhan, please address liggit's comment and sftim's comment |
Co-authored-by: Tim Bannister <tim@scalefactory.com>
Deploy preview for kubernetes-io-vnext-staging processing. Building with commit 2f8d0da https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/60636302d46bf30007023ee9 |
Amended to address comments. |
/lgtm |
LGTM label has been added. Git tree hash: f3f4dd9dcf02b924bc1fde9c51fc7123b97a4b91
|
/lgtm |
/label tide/merge-method-squash |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: reylejano The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Just for the record
registration. Like their stable undeprecated counterparts, deprecated metrics will | ||
be automatically registered to the metrics endpoint and therefore visible. | ||
|
||
On a subsequent release (when the metric's deprecatedVersion is equal to |
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.
nit:
On a subsequent release (when the metric's deprecatedVersion is equal to | |
On a subsequent release (when the metric's `deprecatedVersion` is equal to |
**_Unlike_** their deprecated counterparts, hidden metrics will _no longer_ be | ||
automatically registered to the metrics endpoint (hence hidden). However, they | ||
can be explicitly enabled through a command line flag on the binary | ||
(i.e. `--show-hidden-metrics-for-version=`). This provides cluster admins an |
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.
nit: we avoid Latin abbreviations
(i.e. `--show-hidden-metrics-for-version=`). This provides cluster admins an | |
( `--show-hidden-metrics-for-version=`). This provides cluster admins an |
Thanks @logicalhan |
PR #27358 is a follow-up PR to address #27202 (review) |
Metric stability is now GA, updating deprecation policy accordingly.