Skip to content
This repository has been archived by the owner on Sep 9, 2023. It is now read-only.

WIP: Decorators - Load Balance #204

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fbadiola
Copy link

First implementation draft about decorators.
This draft aims to show a solution to enhance the functionalities of exchanges without modifying the current code and it could live while refactor the other parts.

There's changes on some core files like BasicClient and BasicMutiClient to returns boolean result on unsubscribe methods, like subscription methods do.

Pending:

  • Find good names to each part

  • Define folder structures

  • Move clients to AbstractClient*

  • Testing

  • AbstractClient pretends be like an abstract class on typescript. This is a first iteration to allow reuse more code between core clients and separate their responsabilities. Facade will be the next iteration of the refactor.

Suggestions and criticisms are welcome

Copy link
Member

@bmancini55 bmancini55 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 putting this together and sharing! This technique creates a nice mechanism for a more extensible BasicMultiClient experience. I'm happy to explore this as a replacement for BasicMultiClient till a more robust refactor architecture hammered out. This would necessitate implementing the required strategies for Bibox, CEX, and Coinex to remove the BasicMultiClient code.

The big test is always running subscribeTrades or subscribeLevel2Updates for all markets at one once. That functionality is implemented in the test suite but is only run against a handful of exchanges. During dev, I usually make a script instead of using the suite, as it's easier to debug.

In terms of structure, the only thing that I actively dislike is folder level index.js files.

In terms of using this as a general path forward the biggest issue as we talked about is bubbling up all complexity to the client level. In effect, this code is really just creating strategies for obtaining a socket for a subscription. I'm working on a facade prototype that I'll push up as a WIP PR as well.

Another broader issue is that event relay is going to increase overhead for each message. For high-volume clients (thousands per second in many cases) with a handful of strategies this could be a problem. Right now, the code already performs 3-4 layers of messages passing for a single message event websocket -> smartwss -> client ?-> multiclient -> consumer. In the refactor I'm looking to flatten this and make smartwss functionality adjacent to the socket itself so that it's hopefully just websocket -> client -> consumer.

* Also it have some utils and should had the repeated code across clients.
*/
class AbstractClient extends EventEmitter {
static relay(source, dest, channels = CHANNELS) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice! I like this pattern for forward event emission.

@@ -842,4 +843,4 @@ function candlePeriod(period) {
}
}

module.exports = KucoinClient;
module.exports = LoadBalanceClient.create((...args) => new KucoinClient(...args), { maxSubscriptions: 100 });
Copy link
Member

Choose a reason for hiding this comment

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

Nice clean extensibility of the existing class! 🎉

@fbadiola
Copy link
Author

fbadiola commented Sep 1, 2020

Thanks for putting this together and sharing! This technique creates a nice mechanism for a more extensible BasicMultiClient experience. I'm happy to explore this as a replacement for BasicMultiClient till a more robust refactor architecture hammered out. This would necessitate implementing the required strategies for Bibox, CEX, and Coinex to remove the BasicMultiClient code.

Yes, the next iteration will implement Bibox, Cex and Coinex strategies.

The big test is always running subscribeTrades or subscribeLevel2Updates for all markets at one once. That functionality is implemented in the test suite but is only run against a handful of exchanges. During dev, I usually make a script instead of using the suite, as it's easier to debug.

I do same, use the own library to subscribe all trades. I tested with binance that's exchange with higher frequency events per seconds.

In terms of structure, the only thing that I actively dislike is folder level index.js files.

Why do u dislike it? Anyway, there's not a problem to change it.

In terms of using this as a general path forward the biggest issue as we talked about is bubbling up all complexity to the client level. In effect, this code is really just creating strategies for obtaining a socket for a subscription. I'm working on a facade prototype that I'll push up as a WIP PR as well.

This changes aimn't to solves client responsabilities problem, only abstract some little features that IMHO clients shouldn't know; like max subscriptions or balancing strategies. With barriers we can abstract and iterate this one. Great! I'm waiting for your WIP 👍

Another broader issue is that event relay is going to increase overhead for each message. For high-volume clients (thousands per second in many cases) with a handful of strategies this could be a problem. Right now, the code already performs 3-4 layers of messages passing for a single message event websocket -> smartwss -> client ?-> multiclient -> consumer. In the refactor I'm looking to flatten this and make smartwss functionality adjacent to the socket itself so that it's hopefully just websocket -> client -> consumer.

Yes there's a runtime overhead with multiple layers of EventEmitter, I just made a little benchmark that measure it and give us some vision about overhead. A resume:

  • Three events emitters relayed (with 1e6 iterations): ~ x2,85 slower than a single event emitter
  • Five events emitters relayed (with 1e6 iterations): ~ x4,64 slower than a single event emitter and x1,62 slower than a three events.
    Other approach is using a shared EventEmitter (similar to facade pattern) I just tested in other benchmark too. Client interface could have a getChannel() method where all others decorators use them and they extends an AbstractSharedEventEmitter that meets EventEmitter interface but using the client emitter. This avoid runtime overhead.
    What do u think?

PS: Typescript refactor is must. 🤣

@bmancini55
Copy link
Member

bmancini55 commented Sep 16, 2020

@fbadiola apologies for the delay, I just realized I had not yet replied!

Re index.js, it's mostly a preference. The biggest reason being that outside of the module level it's an extra thing to maintain that is easy to forget about! In the past, it would also break intellisense in VSCode, not sure if it that's still the case though.

Yes there's a runtime overhead with multiple layers of EventEmitter, I just made a little benchmark that measure it and give us some vision about overhead.

Awesome! Very nice work, thanks for testing it out concretely! Something to be mindful of.

I've been playing around with mixins to assist with behavioral extension and have a similar feeling that deeply nested mixin hierarchies would have a similar negative impact on method invocation and property access (mostly based on my understanding of prototypical inheritance).

PS: Typescript refactor is must. rofl

Absolutely! Especially when trying to write any type of composed code.

Speaking of, I still haven't had a chance to put together a complete code example. A few other issue have cropped up to add to the complexity so I'm still hammering through all the variables. The generally idea is that one or more subscriptions gets paired with a live socket. The bundling exists for the duration of the socket. When a socket disconnects, the subscriptions for that socket goes back into a queue to be assigned to a new socket. This ensures reconnects behave the same as initial connections and don't inundate the exchange resulting in cascading failures. So the core client exists to control that process using various strategies. There are a few abstraction points I'm working to define:

  • the pairing strategy that a client uses (single socket for exchange, socket per market, socket per subscription type, number of subscriptions per socket, sockets per time frame)
  • the strategy for activating a socket (simple connection, async requiring a REST token, async requiring a welcome message, async requiring authentication, etc)
  • the strategy for building and sending messages (single message per sub, single message with all subs, etc.)

For instance, for trade subscriptions for an exchange that requires us to send all markets in a single message, we may obtain a socket, debounce for a duration, then construct and send a single message for all markets. This behavior may be totally different for the same exchange when connecting to a private API endpoint to obtain your balance or execute a trade.

Anyways, I digress... will hopefully have something up soon to share! Thanks again for the nice work you've put together and for spending time to help improve this library. Much appreciated!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants