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.

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

metrics: Synchronous EWMA #27918

Closed
wants to merge 4 commits into from
Closed

metrics: Synchronous EWMA #27918

wants to merge 4 commits into from

Conversation

zeim839
Copy link
Contributor

@zeim839 zeim839 commented Aug 13, 2023

Implements #27846

Note:
Updating the EWMA every nanosecond would yield erratically inflated results, especially if Rate() was called immediately after EWMA initialization. I've thus opted for a 1-second interval (allowing uncounted events to accumulate) before updating the EWMA.

Previously, the StandardMeter struct held a snapshot that would be continuously updated by the Tick() function. Since metrics are no longer governed by a clock, I've reworked the Snapshot() function to create a new snapshot struct whenever it is called.

@zeim839 zeim839 force-pushed the ewma_sync branch 2 times, most recently from 3a3971f to c76c6cf Compare August 14, 2023 15:09
@zeim839 zeim839 marked this pull request as draft August 15, 2023 11:20
@zeim839 zeim839 marked this pull request as ready for review August 16, 2023 07:36
@holiman
Copy link
Contributor

holiman commented Oct 10, 2023

Hi, and thanks for your PR.
Unfortunately it came at a unfortunate time, I was working on some older metrics-related PRs and finally realized that the entire metrics-system needed a thorough refactoring, which was implemented in #28035

With that change, this PR became very bitrotted. If you would like to rebase it on top of the changes, we can make another attempt at getting this PR in.

Apologies for not getting to this sooner -- the metrics subsystem/package is a bit of an orphan in go-ethereum, so we don't often prioritize.

@zeim839
Copy link
Contributor Author

zeim839 commented Oct 10, 2023

No problem. I'm not sure if I'll have the time to look into this anytime soon, so I will be closing this PR for the time being.

@zeim839 zeim839 closed this Oct 10, 2023
@zeim839 zeim839 deleted the ewma_sync branch October 10, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants