Track dry-run and apply in metrics by jennybuckley · Pull Request #74997 · kubernetes/kubernetes

jennybuckley · 2019-03-05T21:56:07Z

What type of PR is this?
/kind feature
/sig api-machinery
/wg apply
/priority important-soon

What this PR does / why we need it:
Example value of the metric from kube-up

...
apiserver_request_total{client="kubectl/v0.0.0 (linux/amd64) kubernetes/$Format",code="201",component="apiserver",contentType="application/json",dry_run="All",grou
p="apps",resource="deployments",scope="namespace",subresource="",verb="APPLY",version="v1"} 4
...

Which issue(s) this PR fixes:
Tracked in #73723

Does this PR introduce a user-facing change?:

New "dry_run" metric label (indicating the value of the dryRun query parameter) into the metrics:
* apiserver_request_total
* apiserver_request_duration_seconds
New "APPLY" value for the "verb" metric label which indicates a PATCH with "Content-Type: apply-patch+yaml". This value is experimental and will only be present if the ServerSideApply alpha feature is enabled.

apelisse · 2019-03-05T22:10:45Z

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

Since this is supposed to be a list of strings, and we currently only support one string, maybe you should keep it as the input string?

jpbetz

Generally looks good. Just one observation/question.

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

fejta-bot · 2019-03-05T23:55:53Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

jpbetz

/lgtm

jennybuckley · 2019-03-06T21:02:02Z

I'm not exactly sure why this is considered an API changing PR, I think because I made changes to staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go, but I only changed a function name, so the API is unchanged.
@kubernetes/api-approvers

liggitt · 2019-03-06T21:04:31Z

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/OWNERS#L11 adds the kind/api-change label, which is what the bot watches for

liggitt · 2019-03-06T21:07:52Z

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/instrumentation.md for guidelines, prefer dry_run

I'll update this, thanks. I only put "dryRun" to conform with the existing "contentType" label

I agree that consistency should matter more here.

It looks terrible to have both, but you should probably do what sig instrumentation wants; leave a comment, though.

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

liggitt · 2019-03-06T21:10:39Z

two nits, lgtm otherwise

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

apelisse · 2019-03-07T16:42:23Z

/retest
:-)

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

liggitt · 2019-03-07T21:48:25Z

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

+	if errs := validation.ValidateDryRun(nil, dryRun); len(errs) > 0 {
+		return "invalid"
+	}
+


strings.Join(sets.NewString(dryRun...).List(),",") will dedupe and sort for you if you want to collapse onto that

liggitt · 2019-03-07T21:49:51Z

one comment about the synthetic verb, and one optional comment about using sets.NewString() if you want to use that

~~a good follow up would be to do some sort of cleanup on contentType values as well to reduce cardinality~~ edit: contentType is what we send back, which isn't concerningly high cardinality

jennybuckley · 2019-03-07T22:51:22Z

/retest

liggitt · 2019-03-07T23:17:49Z

/lgtm
/approve

k8s-ci-robot · 2019-03-07T23:18:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jennybuckley, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apimachinery/pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jennybuckley · 2019-03-07T23:45:32Z

/retest

jennybuckley · 2019-03-08T00:31:22Z

/retest

jennybuckley · 2019-03-08T00:47:27Z

/retest

jennybuckley · 2019-03-08T16:43:24Z

/milestone 1.14

k8s-ci-robot · 2019-03-08T16:43:24Z

@jennybuckley: You must be a member of the kubernetes/kubernetes-milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility.

Details

In response to this:

/milestone 1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lavalamp · 2019-03-08T16:54:38Z

/milestone 1.14

k8s-ci-robot · 2019-03-08T16:54:39Z

@lavalamp: The provided milestone is not valid for this repository. Milestones in this repository: [next-candidate, v1.10, v1.11, v1.12, v1.13, v1.14, v1.15, v1.16, v1.17, v1.18, v1.4, v1.5, v1.6, v1.7, v1.8, v1.9]

Use /milestone clear to clear the milestone.

Details

In response to this:

/milestone 1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lavalamp · 2019-03-08T16:54:51Z

/milestone v1.14

jennybuckley · 2019-03-08T20:37:40Z

/retest

k8s-ci-robot requested review from jimmidyson and wojtek-t March 5, 2019 21:57

apelisse reviewed Mar 5, 2019

View reviewed changes

jennybuckley force-pushed the apply-metrics branch from 4bfece2 to 85aabaa Compare March 5, 2019 22:10

jennybuckley mentioned this pull request Mar 5, 2019

Umbrella Issue for Server Side Apply #73723

Closed

jpbetz reviewed Mar 5, 2019

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 5, 2019

jennybuckley force-pushed the apply-metrics branch from 648d3e7 to 8fe34eb Compare March 5, 2019 22:54

k8s-ci-robot assigned jpbetz Mar 6, 2019

jpbetz approved these changes Mar 6, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2019

liggitt reviewed Mar 6, 2019

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2019

jennybuckley force-pushed the apply-metrics branch from d39b0da to 8cc1ff6 Compare March 6, 2019 21:25

lavalamp reviewed Mar 6, 2019

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go Outdated Show resolved Hide resolved

liggitt reviewed Mar 7, 2019

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go Show resolved Hide resolved

liggitt reviewed Mar 7, 2019

View reviewed changes

use utilsets.NewString

077dd28

jennybuckley force-pushed the apply-metrics branch from bffc92e to 077dd28 Compare March 7, 2019 22:20

Feature-gate the APPLY metric value

6e512eb

jennybuckley force-pushed the apply-metrics branch from a3edbb0 to 6e512eb Compare March 7, 2019 23:15

k8s-ci-robot assigned liggitt Mar 7, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2019

k8s-ci-robot added this to the v1.14 milestone Mar 8, 2019

apelisse mentioned this pull request Mar 8, 2019

Dry-run: Path to GA #68514

Closed

10 tasks

This was referenced Mar 8, 2019

Automated cherry pick of #74997: Track dry-run and apply in metrics #75200

Closed

Automated cherry pick of #74997: Track dry-run and apply in metrics #75204

Closed

Automated cherry pick of #74997: Track dry-run and apply in metrics #75207

Closed

k8s-ci-robot merged commit 81e8401 into kubernetes:master Mar 8, 2019

kevindelgado mentioned this pull request Mar 29, 2022

Track field validation in metrics #109120

Merged

Conversation

jennybuckley commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apelisse Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fejta-bot commented Mar 5, 2019

Uh oh!

jpbetz left a comment

Choose a reason for hiding this comment

Uh oh!

jennybuckley commented Mar 6, 2019

Uh oh!

liggitt commented Mar 6, 2019

Uh oh!

liggitt Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

jennybuckley Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

apelisse Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

lavalamp Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liggitt commented Mar 6, 2019

Uh oh!

Uh oh!

apelisse commented Mar 7, 2019

Uh oh!

Uh oh!

liggitt Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

liggitt commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jennybuckley commented Mar 7, 2019

Uh oh!

liggitt commented Mar 7, 2019

Uh oh!

k8s-ci-robot commented Mar 7, 2019

Uh oh!

jennybuckley commented Mar 7, 2019

Uh oh!

jennybuckley commented Mar 8, 2019

Uh oh!

jennybuckley commented Mar 8, 2019

Uh oh!

jennybuckley commented Mar 8, 2019

Uh oh!

k8s-ci-robot commented Mar 8, 2019

Uh oh!

lavalamp commented Mar 8, 2019

Uh oh!

k8s-ci-robot commented Mar 8, 2019

Uh oh!

lavalamp commented Mar 8, 2019

Uh oh!

jennybuckley commented Mar 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jennybuckley commented Mar 5, 2019 •

edited

Loading

apelisse Mar 5, 2019 •

edited

Loading

liggitt commented Mar 7, 2019 •

edited

Loading