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

Add optional input parameter to include all results in the output #231

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

andrestoll
Copy link
Contributor

By setting includeOutputResults to true all stats will be shown in the output.

image

@alexcasalboni
Copy link
Owner

alexcasalboni commented Feb 7, 2024

@andrestoll thanks, this is great 🙏 🚀

I think it'd make sense to remove totalCost from the output, since it doesn't add much value and could be confusing (you need to know the value of num to make sense of it).

For context, totalCost is only used to compute the stateMachine.lambdaCost output (as the total Lambda cost incurred during the power-tuning process).

README.md Outdated
@@ -111,6 +111,7 @@ Power Tuner UI repository: [mattymoomoo/aws-power-tuner-ui](https://github.com/m

From most recent to oldest, with major releases in bold:

* *4.3.4* (2024-02-07): optionally include all results in output
Copy link
Owner

Choose a reason for hiding this comment

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

I'll take care of versioning docs once a new version is pushed out (usually including multiple updates) - we can just remove this for now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

return findOptimalConfiguration(event);
const result = findOptimalConfiguration(event);

if (event.includeOutputResults) {
Copy link
Owner

Choose a reason for hiding this comment

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

It's a very minor change, mainly for consistency with the rest of the codebase, so I'd suggest something like:

if(!!event.includeOutputResults) {

just because I like to always evaluate booleans in if conditions 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@andrestoll
Copy link
Contributor Author

@andrestoll thanks, this is great 🙏 🚀

I think it'd make sense to remove totalCost from the output, since it doesn't add much value and could be confusing (you need to know the value of num to make sense of it).

For context, totalCost is only used to compute the stateMachine.lambdaCost output (as the total Lambda cost incurred during the power-tuning process).

image

@alexcasalboni
Copy link
Owner

I've applied a couple of minor edits.

Merging this now 🚀 🎉

Thank you again for contributing @andrestoll 🙏

@alexcasalboni alexcasalboni merged commit bb8795f into alexcasalboni:master Feb 13, 2024
7 of 8 checks passed
@andrestoll andrestoll deleted the 207-output-stats branch April 22, 2024 12:12
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