Skip to content

inject transformer prefix into metric#80874

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
zshihang:master
Aug 20, 2019
Merged

inject transformer prefix into metric#80874
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
zshihang:master

Conversation

@zshihang
Copy link
Copy Markdown
Contributor

@zshihang zshihang commented Aug 1, 2019 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
this PR injects the transformer prefix into transformer metric so that the metric can give us a sense of the transformers usage.

NONE

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 1, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 1, 2019

Hi @ShiHan9. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@zshihang
Copy link
Copy Markdown
Contributor Author

zshihang commented Aug 1, 2019

/ok-to-test

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 1, 2019

@ShiHan9: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/ok-to-test

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.

@k8s-ci-robot k8s-ci-robot requested review from liggitt and rrati August 1, 2019 22:04
@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 1, 2019
@zshihang
Copy link
Copy Markdown
Contributor Author

zshihang commented Aug 1, 2019 •
edited
Loading

/cc @immutableT
/assign @xiang90

@k8s-ci-robot k8s-ci-robot requested a review from immutableT August 1, 2019 22:06
immutableT
immutableT reviewed Aug 1, 2019
View reviewed changes
staging/src/k8s.io/apiserver/pkg/storage/value/transformer.go Outdated
Copy link
Copy Markdown
Contributor

@immutableT immutableT Aug 1, 2019

Choose a reason for hiding this comment

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

Should not this be moved into the loop?
Otherwise the start time for all transformations will be the same.

Copy link
Copy Markdown
Contributor Author

@zshihang zshihang Aug 1, 2019

Choose a reason for hiding this comment

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

line#146 requires start time being set before for loop. it also makes sense because only one of the transformers will be called.

Copy link
Copy Markdown
Contributor

@immutableT immutableT Aug 1, 2019

Choose a reason for hiding this comment

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

You are correct.

Copy link
Copy Markdown
Contributor

@immutableT immutableT Aug 1, 2019

Choose a reason for hiding this comment

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

/assign @liggitt

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Aug 2, 2019

/cc @logicalhan

at first glance, this looks like an example of #76302

@k8s-ci-robot k8s-ci-robot requested a review from logicalhan August 2, 2019 15:08
liggitt
liggitt reviewed Aug 2, 2019
View reviewed changes
@immutableT
Copy link
Copy Markdown
Contributor

immutableT commented Aug 2, 2019 •
edited
Loading

Reagrding #76302
In practical terms, the number of prefixes (equal the number of providers) is typically one or two.
Though, I agree that this is an unbound label.

Here is the rationale for adding this label:
When migrating from one provider to another it may be useful to track the status of the migration by examining transformation stats per provider.

@logicalhan
Copy link
Copy Markdown

logicalhan commented Aug 2, 2019

Reagrding #76302
In practical terms, the number of prefixes (equal the number of providers) is typically one or two.
Though, I agree that this is an unbound label.

Here is the rationale for adding this label:
When migrating from one provider to another it may be useful to track the status of the migration by examining transformation stats per provider.

I'm personally okay with it if it is practically bounded.

/cc @brancz

@k8s-ci-robot k8s-ci-robot requested a review from brancz August 2, 2019 17:11
@immutableT
Copy link
Copy Markdown
Contributor

immutableT commented Aug 2, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 2, 2019
@zshihang
Copy link
Copy Markdown
Contributor Author

zshihang commented Aug 2, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@brancz
Copy link
Copy Markdown
Member

brancz commented Aug 5, 2019

While in theory unbounded, in practice this is not, so I'd be ok with it.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 16, 2019
immutableT
immutableT reviewed Aug 19, 2019
View reviewed changes
immutableT
immutableT reviewed Aug 19, 2019
View reviewed changes
@zshihang
inject transformer prefix into metric
099484e 
Change-Id: Iacab685a710d8f8d5b80ed0d35e5ccc22bd929cb
@liggitt
Copy link
Copy Markdown
Member

liggitt commented Aug 19, 2019

/approve

will defer to sig-instrumentation for lgtm

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Aug 19, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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 19, 2019
@logicalhan
Copy link
Copy Markdown

logicalhan commented Aug 19, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Aug 19, 2019
@k8s-ci-robot k8s-ci-robot merged commit 9e68e13 into kubernetes:master Aug 20, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 20, 2019
@immutableT immutableT mentioned this pull request Oct 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

@rrati rrati Awaiting requested review from rrati

@logicalhan logicalhan Awaiting requested review from logicalhan

@brancz brancz Awaiting requested review from brancz

+1 more reviewer

@immutableT immutableT immutableT left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@logicalhan logicalhan

@liggitt liggitt

@xiang90 xiang90

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v1.16

Development

Successfully merging this pull request may close these issues.

7 participants

@zshihang @k8s-ci-robot @liggitt @immutableT @logicalhan @brancz @xiang90