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.

Sign up for GitHub

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

Jump to bottom

expands allowed set of packages to define constants that can be verified as part of metric static analysis #103654

Merged
merged 10 commits into from Aug 11, 2021
Merged

expands allowed set of packages to define constants that can be verified as part of metric static analysis #103654

merged 10 commits into from Aug 11, 2021

Conversation

coffeepac
Copy link
Contributor

@coffeepac coffeepac commented Jul 13, 2021 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

expands allowed set of packages to define constants that can be verified as part of metric static analysis

Which issue(s) this PR fixes:

Fixes #

partial fix for #81863

Special notes for your reviewer:

Does this PR introduce a user-facing change?

constants/variables from k8s.io for STABLE metrics is now supported

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet area/test sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 13, 2021
@coffeepac
Copy link
Contributor Author

@logicalhan first cut at this. There are many things left to do. for this PR I want to only include packages from:

  • stdlib
  • k8s.io/kubernetes/pkg
  • k8s.io/kubernetes/staging/src

k8s.io/klog and k8s.io/utils packages are giving me grief, not sure why yet.

I'm leaving packages managed by go itself for now as I need to get a better understanding of how go picks the correct library off disk at compile time to make sure I'm grabbing the right version.

this PR will fail the static analysis check as I didn't update the golden list of stable metrics.

I'm not entirely sure how to create a unit test for this. current unit tests are all done by passing in a string as a code file, my first guess is to create a codefixtures subpackage and put code there or reference existing "live" code as part of the unit test, which is what seems to be happening already.

@logicalhan
Copy link
Member

oh this is kinda awesome, have you tried changing the logic so that we try to parse all metrics instead of just the stable ones?

@logicalhan
Copy link
Member

i'm quite curious how far it gets

@logicalhan
Copy link
Member

/assign

logicalhan
logicalhan reviewed Jul 13, 2021
View reviewed changes
test/instrumentation/decode_metric.go Outdated Show resolved Hide resolved
test/instrumentation/main.go Outdated Show resolved Hide resolved
test/instrumentation/main.go Outdated Show resolved Hide resolved
test/instrumentation/stability-utils.sh Show resolved Hide resolved
@ehashman ehashman added this to Waiting on Author in SIG Node PR Triage Jul 13, 2021
@coffeepac
Copy link
Contributor Author

the verify test failed as expected, I haven't added the demo metric to the stable list (and I shouldn't).

verify failed in a bunch of unexpected ways, I'll sort that out soonish.

@coffeepac
Copy link
Contributor Author

/test pull-kubernetes-node-e2e-containerd

@coffeepac
Copy link
Contributor Author

@logicalhan @brancz this PR adds support to STABLE metrics for including conts/vars from k8s.io (both pkg/ and vendor/) and stdlib. The failing verify test is expected because that metric isn't mine to control but I've marked it as stable to verify the CI.

If you want I can remove support for stdlib, it was easy and I had already done it so I've left it in for now. Otherwise, anything else for this PR?

logicalhan
logicalhan reviewed Jul 29, 2021
View reviewed changes
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what kind of output do you get if you try to get a full list of metrics that are ALPHA?

test/instrumentation/testdata/staging/src/k8s.io/metrics/metrics.go

package metrics

const OKGO = "ThisIsNotTheSoundOfTheTrain"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@ehashman
Copy link
Member

ehashman commented Aug 4, 2021

/cc

@coffeepac
Copy link
Contributor Author

@logicalhan ALPHA output was categorized above #103654 (comment)

@coffeepac
Copy link
Contributor Author

/test pull-kubernetes-node-e2e-containerd

logicalhan
logicalhan reviewed Aug 5, 2021
View reviewed changes
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2021
@coffeepac
Copy link
Contributor Author

/hold cancel

test STABLE metric label reverted to alpha.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2021
logicalhan
logicalhan approved these changes Aug 11, 2021
View reviewed changes
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

This is great! Thanks for tackling this!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2021
@logicalhan
Copy link
Member

/priority important-soon
/triage accepted

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 11, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: coffeepac, logicalhan

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit a878384 into kubernetes:master Aug 11, 2021
SIG Auth Old automation moved this from Needs Triage to Closed / Done Aug 11, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Aug 11, 2021
@ehashman
Copy link
Member

/retitle expands allowed set of packages to define constants that can be verified as part of metric static analysis

@k8s-ci-robot k8s-ci-robot changed the title seems to work, needs tests and a lot of cleanup expands allowed set of packages to define constants that can be verified as part of metric static analysis Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@logicalhan logicalhan logicalhan approved these changes

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

@thockin thockin Awaiting requested review from thockin

@ehashman ehashman Awaiting requested review from ehashman

Assignees

@logicalhan logicalhan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
SIG Node CI/Test Board
Archive-it
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Waiting on Author
SIG Auth Old
Closed / Done
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@coffeepac @k8s-ci-robot @logicalhan @ehashman