search

grafana / xk6-websockets

GNU Affero General Public License v3.0
17 stars 6 forks source link

Switch to fasthttp websocket #47

Closed joeky888 closed 1 year ago

joeky888 commented 1 year ago

Gorilla websocket is deprecated, let's use more maintained fashttp websocket implementation.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

mstoykov commented 1 year ago

Some observations about the library:

  1. it primarily exists to add websocket support to fasthttp server - we do not use that we use the client.
  2. the only changes related to the client seem to be some dependency bumps - we already do that
  3. There is a change from compress/flate to klauspost/compress which might yield some considerable improvements
  4. it still adds fasthttp as a new dependency.

I was going to request some data driven benchmarking on how one of them behaves better then the other but ... I don't see how they will be particularly different :shrug:

I did run the autobahn test and it had exactly the same results so :shrug:.

@olegbespalov do you have anything else to add.

Otherwise, my opinion currently is that if we are going to change from gorilla/websocket I would prefer to go to a library that actually changes stuff in a meaningful direction for us. Before that gorilla/websocket has been practically unmaintained for years. We have had some small problems but this fork does not address any of the ergonomic ones, unlikely to any of the performance ones, and definitely doesn't add any new functionality.

mstoykov commented 1 year ago

Given that gorilla was unarchived I feel like there is no need for this PR at this point.

Thanks for bringing this up @joeky888 :bow: