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

feat: add autocannon-compare-cli package #790

Merged
merged 6 commits into from
Mar 2, 2023

Conversation

guilhermelimak
Copy link
Contributor

@guilhermelimak guilhermelimak commented Feb 15, 2023

Add a small cli utilty to compare autocannon results.

You can see a preview of the comparison here:
image

Closes nearform/hub-draft-issues#172

@github-actions
Copy link

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

Add tests.
Add percentage diff.
Show bytes in a more friendly way.
@guilhermelimak guilhermelimak marked this pull request as ready for review February 16, 2023 04:30
@simoneb
Copy link
Member

simoneb commented Feb 16, 2023

@codeflyer @simone-sanfratello can you please take a look?

@simone-sanfratello
Copy link

Nice! I'd add the option to rename the columns, sometimes it's hard to remember which one is "a" and which one is "b" and it's convenient to have for example "current" and "next"

autocannon-compare-cli -a ./path-to-file-a.json -b ./path-to-file-b.json -a-as current -b-as next

@guilhermelimak guilhermelimak changed the title feat: add initial version of autocannon-compare-cli feat: add autocannon-compare-cli package Feb 16, 2023
Copy link
Member

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

Shall we include using this in the Web UI then?

@guilhermelimak
Copy link
Contributor Author

guilhermelimak commented Feb 16, 2023

Shall we include using this in the Web UI then?

I think that's a good idea. Should make it way easier to use it that way.

@simone-sanfratello
Copy link

Shall we include using this in the Web UI then?

That was my original request! 🚀

Create new compare route on backend.
Refactor and polishing.
@guilhermelimak
Copy link
Contributor Author

I've updated the backend with a new route to return the new comparison, but the frontend changes are still missing.

@guilhermelimak
Copy link
Contributor Author

This now is partially implemented on the frontend, it's using the new route and showing the results in a table but could use some improvements on styling like adding color to the results.

I kept the old compare route intact and created a v2 using the new compare function. I'm not entirely sure if we should try to integrate both the new and old results since they carried some different info on them. If that's not desirable we could just remove the old one and remove the v2 naming from the new one.

@simoneb
Copy link
Member

simoneb commented Feb 27, 2023

@guilhermelimak where do we stand with this PR?

@guilhermelimak
Copy link
Contributor Author

@simoneb should be ready for reviewing and testing, there's the point I mentioned on my previous comment about some data we had on the previous compare function and we don't have on the new one but should be good to go if that's fine

@simoneb simoneb enabled auto-merge (squash) March 2, 2023 12:41
@simoneb simoneb merged commit b936fa5 into master Mar 2, 2023
@simoneb simoneb deleted the feat/add-autcoannon-compare-cli branch March 2, 2023 12:42
@github-actions github-actions bot mentioned this pull request Mar 7, 2023
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