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

Implement scaling latency metrics through logical clock #983

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Omrigan
Copy link
Contributor

@Omrigan Omrigan commented Jun 20, 2024

Fixes #594.

@Omrigan
Copy link
Contributor Author

Omrigan commented Jun 20, 2024

@sharnoff can you take a look, if you have a moment? This is very WIP, I didn't update the tests or tested it.

Looking at the logic around agent's state -> does this logic of clock propagation looks sound to you? Can I make it simpler and more compact somehow?

Copy link
Member

@sharnoff sharnoff left a comment

Choose a reason for hiding this comment

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

Had a look - I think I'm struggling to get the big picture of what the intended flow within the autoscaler-agent state machine is (mostly because the existing code is quite complicated, and the new stuff doesn't yet have comments).

Broadly I think it should be ok, but do you have a quick (1-2 paragraph) explanation of how the clocks are supposed to flow?

(IIUC, currently the "desired logical time" stored in the plugin/monitor/neonvm state is basically the logical time of the most recent scaling that the component has completed successfully? If so, AFAICT there are still some subtle edge cases, but they shouldn't require major changes to accommodate.)

Would be good to discuss on Monday 😅


All that aside, one thing I noticed: In a lot of places, there's variables like desiredClock or desiredLogicalTime -- IMO, this reads with desired as an adjective affecting the clock/logical time, which I guess is not what's intended. I wonder if it'd be better to refer to these more like timestamps, e.g. "tsOfDesired" or "desiredAtTime" etc. (or event just "desiredAt"?)

Signed-off-by: Oleg Vasilev <[email protected]>
Signed-off-by: Oleg Vasilev <[email protected]>
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.

Epic: Scaling latency metrics
2 participants