Skip to content
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

Track dry-run and apply in metrics #74997

Merged
merged 4 commits into from Mar 8, 2019

Conversation

@jennybuckley
Copy link
Contributor

commented Mar 5, 2019

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.
return reportedVerb
}

func cleanDryRun(dryRun []string) string {
if len(dryRun) != 0 {

This comment has been minimized.

Copy link
@apelisse

apelisse Mar 5, 2019

Member

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?

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-metrics branch from 4bfece2 to 85aabaa Mar 5, 2019
@jennybuckley jennybuckley referenced this pull request Mar 5, 2019
45 of 82 tasks complete
Copy link
Contributor

left a comment

Generally looks good. Just one observation/question.

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-metrics branch from 648d3e7 to 8fe34eb Mar 5, 2019
@fejta-bot

This comment has been minimized.

Copy link

commented Mar 5, 2019

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
jpbetz approved these changes Mar 6, 2019
Copy link
Contributor

left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 6, 2019
@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

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

},
[]string{"verb", "group", "version", "resource", "subresource", "scope", "component", "client", "contentType", "code"},
[]string{"verb", "dryRun", "group", "version", "resource", "subresource", "scope", "component", "client", "contentType", "code"},

This comment has been minimized.

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Mar 6, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@apelisse

apelisse Mar 6, 2019

Member

I agree that consistency should matter more here.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 6, 2019

Member

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

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

two nits, lgtm otherwise

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 6, 2019
@jennybuckley jennybuckley force-pushed the jennybuckley:apply-metrics branch from d39b0da to 8cc1ff6 Mar 6, 2019
@apelisse

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

/retest
:-)

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

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 7, 2019

Member

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

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

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 jennybuckley force-pushed the jennybuckley:apply-metrics branch from bffc92e to 077dd28 Mar 7, 2019
@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

/retest

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-metrics branch from a3edbb0 to 6e512eb Mar 7, 2019
@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

[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

Needs approval from an approver in each of these files:

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

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

/retest

2 similar comments
@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

/retest

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

/retest

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

/milestone 1.14

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@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.

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

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

/milestone 1.14

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@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.

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

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

/milestone v1.14

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 81e8401 into kubernetes:master Mar 8, 2019
17 checks passed
17 checks passed
cla/linuxfoundation jennybuckley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.