Skip to content

feat: improved logging, scaling and documentation #23

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

Merged
merged 13 commits into from
May 22, 2025

Conversation

PeterBaker0
Copy link
Collaborator

@PeterBaker0 PeterBaker0 commented May 19, 2025

Improves the capacity manager implementation with

  • revised config parsing approach to not dynamically build zod model, but instead pull out env variables directly, and handle parsing into number where needed directly from the string environment, while still getting type hinting in the construction of the object for parsing by manipulating assumed type of the environment
  • logging using winston in all methods, much more thorough logging for debugging as needed
  • documentation/doc string improvements
  • implemented logarithmic based scaling algorithm with controls for sensitivity + scaling factor, allowing a range of scaling behaviours, found parameters which suit our needs but can be configured
  • improved management of scaling by tracking pending tasks as they start up, keeping this in sync as we poll - pending tasks are counted as towards the target task count based on the scaling function

Other changes

Also updates job interfaces to add wave height/period and make inputs optional - see open-AIMS/ReefGuideAPI.jl#67

Could do later

  • expose new scaling config in the JSON config
  • tidy up a few parts of the manager i.e. still not super super happy with the env parsing approach - it'd probably be nicer to make the env strictly string and then transform types using zod using the .transform method
  • tooling around the scaling function to help determine suitable values against target thresholds, or to at least graph it

Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Signed-off-by: Peter Baker <peter.baker122@csiro.au>
…n task types to track counts

Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Copy link

❌ Code formatting check failed. Please run npm run format:write locally and commit the changes.

Signed-off-by: Peter Baker <peter.baker122@csiro.au>
@PeterBaker0
Copy link
Collaborator Author

PeterBaker0 commented May 19, 2025

https://www.desmos.com/calculator/h2hxuqi2gk

Can be used to play with scaling function params

@PeterBaker0 PeterBaker0 requested a review from arlowhite May 19, 2025 06:15
PeterBaker0 and others added 4 commits May 19, 2025 16:17
Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Copy link
Contributor

@arlowhite arlowhite 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. Ideally we'd have test coverage of this code, but it's tricky. At some point, I'd like to discuss if we want to create tests for this and how to approach it


try {
// Build set of distinct worker types from the tracked workers
const clusterArns = new Set(this.trackedWorkers.map(w => w.clusterArn));
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally would have tests for these functions, but they're not so easy to test, would need to mock things. maybe an integration test? we can discuss testing approaches later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah - there are possible options - but agree it's a bit tricky

Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Signed-off-by: Peter Baker <peter.baker122@csiro.au>
Copy link

❌ Code formatting check failed. Please run npm run format:write locally and commit the changes.

@PeterBaker0 PeterBaker0 merged commit c4c324a into main May 22, 2025
2 checks passed
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