Skip to content

Update metrics.rs #5215

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

Conversation

DoTheBestToGetTheBest
Copy link

related with #5206

i adjusted for a range up to 0.1 seconds (100ms)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@dapplion
Copy link
Collaborator

dapplion commented Feb 8, 2024

Metrics from the original issue show that the lower resolution is not being used. Not sure @michaelsproul if you need 5ms resolution, otherwise the metric lin range could be of resolution 10ms and range up to 200ms

linear_buckets(0.01, 0.01, 20)

@michaelsproul
Copy link
Member

yeah IMO 10ms would be fine, I would also prefer less buckets (10-15, not 20)

@michaelsproul michaelsproul added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Feb 8, 2024
@michaelsproul michaelsproul changed the base branch from stable to unstable February 8, 2024 03:48
@michaelsproul
Copy link
Member

@DoTheBestToGetTheBest you also need to accept the CLA

@DoTheBestToGetTheBest
Copy link
Author

Metrics from the original issue show that the lower resolution is not being used. Not sure @michaelsproul if you need 5ms resolution, otherwise the metric lin range could be of resolution 10ms and range up to 200ms

linear_buckets(0.01, 0.01, 20)

thank you for your review!

yeah IMO 10ms would be fine, I would also prefer less buckets (10-15, not 20)

thank you for your review! changed to 10

@DoTheBestToGetTheBest
Copy link
Author

@DoTheBestToGetTheBest you also need to accept the CLA

what is this?

@michaelsproul
Copy link
Member

michaelsproul commented Feb 9, 2024

It's a contributor license agreement, it just says that you agree to license your contributions under the same license as the rest of Lighthouse (Apache 2.0). It's a bit of a bureaucratic hurdle, but only takes a second, see: #5215 (comment).

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Thanks! 15 should be ok

@chong-he
Copy link
Member

@DoTheBestToGetTheBest you also need to accept the CLA

what is this?

At the bottom of this PR, or the direct link: https://cla-assistant.io/sigp/lighthouse?pullRequest=5215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants