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

Adding dry run option to request, allows requests to be validated and for an estimated cost to be returned, prior to full generation #35

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johnsabath
Copy link
Contributor

No description provided.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@@ -286,6 +286,7 @@ message Answer {
uint64 created = 4;
optional AnswerMeta meta = 6;
repeated Artifact artifacts = 7;
optional double estimated_cost = 8; // Only set if Request.dry_run is true.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm partial to just calling this cost which would allow it to be used both for dry_run estimates as well as actual cost if desired. dry_run already implies estimation / non-actual results.

Copy link
Contributor

Choose a reason for hiding this comment

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

Units for this need to be documented somewhere as well ($ or credits).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm partial to just calling this cost which would allow it to be used both for dry_run estimates as well as actual cost if desired. dry_run already implies estimation / non-actual results.

I like that.

Units for this need to be documented somewhere as well ($ or credits).

Definitely. I'm waiting for feedback on which units we want to use here, so it's deliberately vague at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want credits here. And we should be able to return the cost of a Request in an Answer regardless of whether or not dry run is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely could.

Do y'all feel that in a multi-answer scenario, returning the cost on each Answer could be confusing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think returning the cost with the respective answer would be the appropriate way of doing it and less confusing than returning the cost elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where I see the confusion coming from is that we're computing the cost for a single request, but returning multiple answers.

Do the answers report their share of the cost, or do they report the total cost of the request?

Copy link
Contributor

@arsenetar arsenetar Nov 9, 2022

Choose a reason for hiding this comment

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

I suppose regardless of the exact desired behavior the cost needs to be within an answers object since that is the message used for the responses. Either each answer could have partial cost, or the total cost could be reported by the last answer sent back to the client. I think either one could be appropriate. Regardless of that implementation detail, I think we want to move forward with labeling this cost and per other discussions using credits as the unit of measure here.

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.

3 participants