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

rework for go-chi #23

Merged
merged 3 commits into from
Jun 17, 2021
Merged

Conversation

bigangryrobot
Copy link
Contributor

Copy link
Contributor

@xxx7xxxx xxx7xxxx left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. It's totally OK to use a new web framework to handle admin API. But there's one thing I'm considering. The chi looks like a little low level by comparing with echo[1], which tries to be compatible with stdlib. How do you consider it?

[1] https://github.com/labstack/echo

pkg/api/server.go Outdated Show resolved Hide resolved
cmd/server/main.go Outdated Show resolved Hide resolved
pkg/api/api.go Outdated Show resolved Hide resolved
pkg/api/middlewares.go Outdated Show resolved Hide resolved
pkg/api/object.go Outdated Show resolved Hide resolved
@bigangryrobot
Copy link
Contributor Author

good comments thank you both, let me clean all this up. also, ya echo would be fine and maybe thats a smaller code change overall, let me review that and see how it works out

@bigangryrobot
Copy link
Contributor Author

Ok folks, that should address the comments you gave. Im not sure I like the logging implementation, the http helpers and lack of http tests, so I might tackle that next

pkg/api/middlewares.go Outdated Show resolved Hide resolved
@bigangryrobot
Copy link
Contributor Author

bigangryrobot commented Jun 17, 2021

For a performance comparison, here is a quick look using echo "GET http://localhost:2381/apis/v1/status/members"| vegeta attack -duration=25s | tee results.bin | vegeta report

Iris:

Requests      [total, rate, throughput]         1250, 50.04, 50.04
Duration      [total, attack, wait]             24.982s, 24.981s, 1.765ms
Latencies     [min, mean, 50, 90, 95, 99, max]  803.839µs, 2.136ms, 2.138ms, 2.612ms, 2.757ms, 6.506ms, 17.971ms
Bytes In      [total, mean]                     952500, 762.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      200:1250

Chi:

Requests      [total, rate, throughput]         1250, 50.04, 50.04
Duration      [total, attack, wait]             24.982s, 24.98s, 1.725ms
Latencies     [min, mean, 50, 90, 95, 99, max]  701.835µs, 2.154ms, 2.153ms, 2.582ms, 2.654ms, 7.62ms, 20.543ms
Bytes In      [total, mean]                     952500, 762.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      200:1250

And for echo "GET http://localhost:2381/apis/v1/objects"| vegeta attack -duration=25s | tee results.bin | vegeta report

Iris:

Requests      [total, rate, throughput]         1250, 50.04, 50.03
Duration      [total, attack, wait]             24.983s, 24.98s, 3.657ms
Latencies     [min, mean, 50, 90, 95, 99, max]  2.914ms, 5.539ms, 5.59ms, 6.504ms, 6.709ms, 9.21ms, 22.104ms
Bytes In      [total, mean]                     1667500, 1334.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      200:1250

Chi:

Requests      [total, rate, throughput]         1250, 50.04, 50.03
Duration      [total, attack, wait]             24.983s, 24.98s, 3.33ms
Latencies     [min, mean, 50, 90, 95, 99, max]  2.98ms, 5.553ms, 5.57ms, 6.426ms, 6.657ms, 9.06ms, 21.311ms
Bytes In      [total, mean]                     1667500, 1334.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      200:1250

@bigangryrobot
Copy link
Contributor Author

bigangryrobot commented Jun 17, 2021

i guess i didnt try add r.ContentLength last night 🤦 , but i added it now and it seems to work just fine so that should wrap up the logger

Copy link
Contributor

@benja-wu benja-wu left a comment

Choose a reason for hiding this comment

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

LGTM

@benja-wu benja-wu merged commit f1f032d into easegress-io:main Jun 17, 2021
@bigangryrobot
Copy link
Contributor Author

its worth mentioning that I did roll through 80% of adding the echo framework as well, just to see how it went and it was a bit harder to implement due mainly to how the easegress framework is currently structured. It might be something to return to after some time.

@benja-wu
Copy link
Contributor

benja-wu commented Jun 17, 2021

its worth mentioning that I did roll through 80% of adding the echo framework as well, just to see how it went and it was a bit harder to implement due mainly to how the easegress framework is currently structured. It might be something to return to after some time.

@bigangryrobot Thank you so much for contributing this PR and providing a performance quick look. The echo is also a cool web framework but I think go-chi looks good enough for Easegress API management currently.

Feel free to open an issue about bug reportings, design discussions, or new feature proposals.

@xxx7xxxx
Copy link
Contributor

Sure, I just think echo got higher abstraction than chi in some ways. But chi is fine in this simple scenario. Thanks for your contribution!

@benja-wu benja-wu added this to the v1.0.1 milestone Jun 18, 2021
fengyie007 pushed a commit to fengyie007/easegress that referenced this pull request Jun 21, 2021
* rework for go-chi

* cleanup chi implementation

* add r.ContentLength to logger

Co-authored-by: cbeverlin <cbeverlin@tesla.com>
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