Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goodput initial implementation #32
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
Goodput initial implementation #32
Changes from 5 commits
4b132a4
22eaaea
8f5f189
5d01561
5f46340
baa1d6d
bd48def
442b77b
d154afb
cfa1de2
88678b3
7ccbbc2
3e20eac
beef601
73c770b
9636153
2b3f267
be076d7
8d7abfc
b97b668
0232c08
f0e9faa
b170345
222d1dd
c34f9db
afa2784
a3ab155
274a0f2
f3c509f
9eaaa81
994b5b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove any options that are just stating the default values here. Less code to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are outputting the raw value here.
Do we also want to include a percentage? To give the user of a quick sense of the quality of the run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we make the value 0 in these cases? Then the code could be refactored to avoid this unique logic handling and continue. It would also make parsing a bit easier, because all of the parsed values would be ints or floats.
For example:
Then, you could set the value to 0 somewhere else if there's an error. You should log that error in the same place, if you're not already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please go through the code and remove the use of "SLOs". Someone then has to figure out what SLOs are if they're not already familiar. You can use a different term like "constraints" that are more universally understood.