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

UsedVolumesSize and UsedStoreInBytes metrics #155

Merged
merged 19 commits into from
Feb 8, 2022
Merged

UsedVolumesSize and UsedStoreInBytes metrics #155

merged 19 commits into from
Feb 8, 2022

Conversation

klapkov
Copy link
Contributor

@klapkov klapkov commented Dec 1, 2021

No description provided.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/180493937

The labels on this github issue will be updated when the story is started.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 1, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@MarcPaquette
Copy link
Contributor

MarcPaquette commented Dec 7, 2021 •
edited
Loading

This PR is related to this issue: cloudfoundry/garden-runc-release#210

@MarcPaquette MarcPaquette mentioned this pull request Dec 7, 2021
Closed
@MarcPaquette
Copy link
Contributor

An internal tracking story has been created for this PR here: https://www.pivotaltracker.com/story/show/180559618

@MarcPaquette
Copy link
Contributor

MarcPaquette commented Dec 20, 2021 •
edited
Loading

Hi @klapkov
I have an issue logged in our backlog to have someone take a look at your PR. At this time we are focusing on getting many of the Runtime resources bumped to golang 1.17. That, coupled with the end of year holiday season, is slowing down our progress some.

Some quick feedback would be to squash the commits in the PR.

Thanks,
@MarcPaquette

MarcPaquette
MarcPaquette reviewed Feb 1, 2022
View reviewed changes
groot/cleaner.go Outdated
"time"
"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Double fmt here. Could you remove one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing it right away.

MarcPaquette
MarcPaquette reviewed Feb 1, 2022
View reviewed changes
Copy link
Contributor

@MarcPaquette MarcPaquette left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All these changes look good.

I'm going to confirm these changes with our PM.

@MarcPaquette
Copy link
Contributor

MarcPaquette commented Feb 7, 2022 •
edited
Loading

Reviewed these changes with PM and Designer. Doing one final test per their feedback, then merging in if all goes well.

@MarcPaquette
Copy link
Contributor

Hi @klapkov

The metrics being emitted are being converted into a number that's non-human readable. Would converting to MB or KB make more sense in this context given the sizes that may be present?

origin:"grootfs" eventType:ValueMetric timestamp:1644254494727066562 deployment:"cf" job:"diego-cell" index:"fd8b68ec-7582-4733-9a35-b8cbd1a890e7" ip:"10.0.1.11" tags:<key:"source_id" value:"grootfs" > valueMetric:<name:"UsedBackingStoreInBytes" value:2.700161024e+09 unit:"bytes" >
origin:"grootfs" eventType:ValueMetric timestamp:1644254494745729516 deployment:"cf" job:"diego-cell" index:"fd8b68ec-7582-4733-9a35-b8cbd1a890e7" ip:"10.0.1.11" tags:<key:"source_id" value:"grootfs" > valueMetric:<name:"UsedLayersSize" value:1.058217682e+09 unit:"bytes" >
origin:"grootfs" eventType:ValueMetric timestamp:1644254495438149477 deployment:"cf" job:"diego-cell" index:"fd8b68ec-7582-4733-9a35-b8cbd1a890e7" ip:"10.0.1.11" tags:<key:"source_id" value:"grootfs" > valueMetric:<name:"UsedLayersSize" value:1.058217682e+09 unit:"bytes" >

Thanks,
Marc

@klapkov
Copy link
Contributor Author

klapkov commented Feb 8, 2022

Hi @MarcPaquette,
Yes indeed currently they aren't non-human readable. When implementing the metrics, I noticed that all of the other ones are also emitted in bytes and just decided that the best choice is to follow the convention, but honestly from my point of view it doesn't matter if they are emitted in MB or KB. So I leave this decision to you guys. Should I change them ?

@MarcPaquette
Copy link
Contributor

Hi @klapkov

I don't have strong feelings either way. We can always change it down the line fairly easily.

Thanks for the contribution and your patience!

@MarcPaquette MarcPaquette merged commit de96e9e into cloudfoundry:master Feb 8, 2022
PlamenDoychev added a commit to PlamenDoychev/community that referenced this pull request Mar 9, 2024
@PlamenDoychev
Add klapkov as ARP Reviewer
448a2ff 
[DIEGO-RELEASE] Add locket client keepalive time and timeout to jobs: cloudfoundry/diego-release#722
[DIEGO-RELEASE] Make max_containers prop configurable: cloudfoundry/diego-release#876
[BBS] Add request metrics for BBS - still open: cloudfoundry/bbs#80
[BBS] Use scheduling info instead of the whole desiredLRP: cloudfoundry/bbs#79
[BBS] Add routing info endpoint: 
cloudfoundry/bbs#66
[BBS] Remove cpu_weight limit: 
cloudfoundry/bbs#81
[EXECUTOR] Improve error handling on process start - still open: cloudfoundry/executor#91
[LOCKET]  Add a keepalive timeout on the locket client: cloudfoundry/locket#12
[GROOTFS] UsedVolumesSize and UsedStoreInBytes metrics: cloudfoundry/grootfs#155
[ROUTE-EMITTER] Use routing info bbs endpoint when syncing: cloudfoundry/route-emitter#23
[ROUTE-EMITTER]  Use routing_info for desired_lrp's when there are missing actual_lrp's: 
cloudfoundry/route-emitter#26
[SILK-RELEASE] Deduplicate Iptables Rules with Dynamic ASG's: cloudfoundry/silk-release#101
[SILK-RELEASE] Make container_metadata_file_check_timeout on silk-shutdown configurable: 
cloudfoundry/silk-release#111
@PlamenDoychev PlamenDoychev mentioned this pull request Mar 9, 2024
Closed
ameowlia pushed a commit to cloudfoundry/community that referenced this pull request Mar 12, 2024
@PlamenDoychev @ameowlia
Add klapkov as ARP Reviewer
3b4e19d 
[DIEGO-RELEASE] Add locket client keepalive time and timeout to jobs: cloudfoundry/diego-release#722
[DIEGO-RELEASE] Make max_containers prop configurable: cloudfoundry/diego-release#876
[BBS] Add request metrics for BBS - still open: cloudfoundry/bbs#80
[BBS] Use scheduling info instead of the whole desiredLRP: cloudfoundry/bbs#79
[BBS] Add routing info endpoint:
cloudfoundry/bbs#66
[BBS] Remove cpu_weight limit:
cloudfoundry/bbs#81
[EXECUTOR] Improve error handling on process start - still open: cloudfoundry/executor#91
[LOCKET]  Add a keepalive timeout on the locket client: cloudfoundry/locket#12
[GROOTFS] UsedVolumesSize and UsedStoreInBytes metrics: cloudfoundry/grootfs#155
[ROUTE-EMITTER] Use routing info bbs endpoint when syncing: cloudfoundry/route-emitter#23
[ROUTE-EMITTER]  Use routing_info for desired_lrp's when there are missing actual_lrp's:
cloudfoundry/route-emitter#26
[SILK-RELEASE] Deduplicate Iptables Rules with Dynamic ASG's: cloudfoundry/silk-release#101
[SILK-RELEASE] Make container_metadata_file_check_timeout on silk-shutdown configurable:
cloudfoundry/silk-release#111

wip
ameowlia pushed a commit to cloudfoundry/community that referenced this pull request Mar 12, 2024
@PlamenDoychev @ameowlia
Add klapkov as ARP Reviewer
18591a5 
[DIEGO-RELEASE] Add locket client keepalive time and timeout to jobs: cloudfoundry/diego-release#722
[DIEGO-RELEASE] Make max_containers prop configurable: cloudfoundry/diego-release#876
[BBS] Add request metrics for BBS - still open: cloudfoundry/bbs#80
[BBS] Use scheduling info instead of the whole desiredLRP: cloudfoundry/bbs#79
[BBS] Add routing info endpoint:
cloudfoundry/bbs#66
[BBS] Remove cpu_weight limit:
cloudfoundry/bbs#81
[EXECUTOR] Improve error handling on process start - still open: cloudfoundry/executor#91
[LOCKET]  Add a keepalive timeout on the locket client: cloudfoundry/locket#12
[GROOTFS] UsedVolumesSize and UsedStoreInBytes metrics: cloudfoundry/grootfs#155
[ROUTE-EMITTER] Use routing info bbs endpoint when syncing: cloudfoundry/route-emitter#23
[ROUTE-EMITTER]  Use routing_info for desired_lrp's when there are missing actual_lrp's:
cloudfoundry/route-emitter#26
[SILK-RELEASE] Deduplicate Iptables Rules with Dynamic ASG's: cloudfoundry/silk-release#101
[SILK-RELEASE] Make container_metadata_file_check_timeout on silk-shutdown configurable:
cloudfoundry/silk-release#111

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

@MarcPaquette MarcPaquette MarcPaquette left review comments

Assignees

@MarcPaquette MarcPaquette

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@klapkov @cf-gitbot @MarcPaquette @gcapizzi