Skip to content

feature: add gc scheduler metrics collection count#5263

Merged
fuweid merged 1 commit intocontainerd:mainfrom
pacoxu:add-metrics-scheduler
Nov 11, 2022
Merged

feature: add gc scheduler metrics collection count#5263
fuweid merged 1 commit intocontainerd:mainfrom
pacoxu:add-metrics-scheduler

Conversation

@pacoxu
Copy link
Copy Markdown
Contributor

@pacoxu pacoxu commented Mar 25, 2021 •
edited
Loading

Fix todo

containerd/gc/scheduler/scheduler.go

Line 258 in 6948d89

// TODO(dmcg): expose collection stats as metrics

With these metrics, we can define an alert in Prometheus which is the consumer of metrics. For instance,

  • alert if the GC keeps failing(N times).
  • alert if the GC duration is above a bar(maybe 15s/1m) for N times.
    Just something we can do if we have such metrics.

I find this to-do in comments, and not in a true issue that needs it. So I have no accurate scenarios that would use these metrics.

See #5263 (comment)

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Mar 25, 2021

Hi @pacoxu. Thanks for your PR.

I'm waiting for a containerd 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.

@pacoxu pacoxu changed the title add gc scheduler metrics: collection count [wip]add gc scheduler metrics: collection count Mar 25, 2021
@pacoxu pacoxu changed the title [wip]add gc scheduler metrics: collection count add gc scheduler metrics: collection count Mar 25, 2021
@pacoxu pacoxu force-pushed the add-metrics-scheduler branch from 985cd1b to ee2ac69 Compare March 25, 2021 05:14
@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Mar 25, 2021 •
edited
Loading

need upgrade prometheus client_golang first. Run go mod vendor

Updated

@pacoxu pacoxu force-pushed the add-metrics-scheduler branch from 6fae444 to 1eadf5a Compare March 25, 2021 06:17
@pacoxu pacoxu changed the title add gc scheduler metrics: collection count feature: add gc scheduler metrics- collection count Mar 26, 2021
@kzys
Copy link
Copy Markdown
Member

kzys commented May 27, 2021

Seems your commit subject (the first line of the commit message) is too long. Please take a look https://github.com/containerd/containerd/pull/5263/checks?check_run_id=2190784348.

kzys
kzys reviewed May 27, 2021
View reviewed changes
Comment thread gc/scheduler/scheduler.go Outdated
kzys
kzys reviewed May 27, 2021
View reviewed changes
Comment thread gc/scheduler/metrics.go Outdated
@kzys
Copy link
Copy Markdown
Member

kzys commented Jul 2, 2021

@pacoxu I'm interested to have this metrics in 1.6 if possible. Could you update the PR?

@pacoxu pacoxu force-pushed the add-metrics-scheduler branch from 1eadf5a to 79b66ea Compare July 5, 2021 02:31
@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Jul 5, 2021

Updated per your comments. @kzys

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 5, 2021

Build succeeded.

@pacoxu pacoxu requested a review from kzys July 5, 2021 02:33
kzys
kzys approved these changes Jul 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@containerd/committers Can someone take a look?

@kzys
Copy link
Copy Markdown
Member

kzys commented Jul 22, 2021

/ok-to-test

@kzys
Copy link
Copy Markdown
Member

kzys commented Jul 22, 2021

@pacoxu Please use your real name on the signed-off line.

@pacoxu pacoxu force-pushed the add-metrics-scheduler branch from 79b66ea to 5f5141a Compare July 26, 2021 04:05
@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Jul 26, 2021

ok @kzys thanks for your review.
Updated

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 26, 2021

Build succeeded.

@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Sep 7, 2021

/cc @dmcgowan @mikebrow
kindly ping as there is no update for more than a month.

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Sep 7, 2021

please rebase to make sure that CI is happy. thanks 🙏

@pacoxu pacoxu force-pushed the add-metrics-scheduler branch from 5f5141a to 617ce04 Compare September 7, 2021 11:23
@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Sep 7, 2021

please rebase to make sure that CI is happy. thanks 🙏

@fuweid rebased and needs to wait for CI running.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Sep 7, 2021

Build succeeded.

@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Oct 5, 2021

Not sure if it's a mandate, but other metrics in the repo are using docker/go-metrics for consistent naming.

1 similar comment
@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Oct 5, 2021

Not sure if it's a mandate, but other metrics in the repo are using docker/go-metrics for consistent naming.

@pacoxu pacoxu force-pushed the add-metrics-scheduler branch from 617ce04 to 966f4af Compare June 6, 2022 10:22
@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Jun 6, 2022

Not sure if it's a mandate, but other metrics in the repo are using docker/go-metrics for consistent naming.

replace the implementation. Updated.

@jonyhy96 jonyhy96 mentioned this pull request Jun 6, 2022
Closed
@pacoxu pacoxu force-pushed the add-metrics-scheduler branch from 966f4af to 012d68f Compare June 7, 2022 02:39
@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Aug 19, 2022

@cpuguy83 do you have time to take a look again?

@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Nov 7, 2022

ack
/cc @samuelkarp @ruiwen-zhao
as you are reviewing my other metrics PR.

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Nov 7, 2022

@pacoxu: GitHub didn't allow me to request PR reviews from the following users: ruiwen-zhao.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

ack
/cc @samuelkarp @ruiwen-zhao
as you are reviewing my other metrics PR.

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.

@ruiwen-zhao
Copy link
Copy Markdown
Member

ruiwen-zhao commented Nov 8, 2022

/lgtm

Non blocking and for my own education: how will the new metrics be used? More specifically, what issues will the new metrics help debug and how will they help?

@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Nov 8, 2022

Non blocking and for my own education: how will the new metrics be used? More specifically, what issues will the new metrics help debug and how will they help?

With these metrics, we can define an alert in Prometheus which is the consumer of metrics. For instance,

  • alert if the GC keeps failing(N times).
  • alert if the GC duration is above a bar(maybe 15s/1m) for N times.

Just something we can do if we have such metrics.

I find this to-do in comments, and not in a true issue that needs it. So I have no accurate scenarios that would use these metrics. 😂

@pacoxu pacoxu changed the title feature: add gc scheduler metrics- collection count feature: add gc scheduler metrics collection count Nov 10, 2022
@fuweid fuweid merged commit f226fa8 into containerd:main Nov 11, 2022
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Nov 11, 2022

Thanks @pacoxu for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kzys kzys kzys approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

@fuweid fuweid fuweid approved these changes

@mikebrow mikebrow Awaiting requested review from mikebrow

@samuelkarp samuelkarp Awaiting requested review from samuelkarp

Assignees

No one assigned

Labels

impact/changelog ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pacoxu @k8s-ci-robot @kzys @fuweid @cpuguy83 @ruiwen-zhao @dmcgowan