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: introduce queue ✨ #464

Merged
merged 12 commits into from
Aug 14, 2022

Conversation

habibalkhabbaz
Copy link
Contributor

@habibalkhabbaz habibalkhabbaz commented Aug 10, 2022

Description

This PR introduces the queue feature

Related Issue

#451
#463

Motivation and Context

After moving to WebSocket we encounter some small issues regarding the trailingTrade execution, we start thinking about using a queue to improve the bot #448

How Has This Been Tested?

Screenshots (if appropriate):

@habibalkhabbaz
Copy link
Contributor Author

@chrisleekr
It would be nice if you can test the performance of this PR in your Rasberry Pi because I don't have one xD

@uhliksk
I added a fix for #463, you can test and check if it's solved.

@uhliksk
Copy link
Contributor

uhliksk commented Aug 11, 2022

@habibalkhabbaz Nice job. I'll try. Thank you.

@chrisleekr Please merge this #464 before my #462 as I can use this queueing mechanism instead of my simple queueing to solve #398 and #457. Thank you.

@uhliksk
Copy link
Contributor

uhliksk commented Aug 11, 2022

@habibalkhabbaz Logging is working fine.

image

@habibalkhabbaz
Copy link
Contributor Author

@habibalkhabbaz Logging is working fine.

image

Perfect!

@uhliksk
Copy link
Contributor

uhliksk commented Aug 11, 2022

@habibalkhabbaz Is this because of racing the queue?

{"name":"binance-api","version":"0.0.88","hostname":"f545a1de3f6a","pid":46,"gitHash":"unspecified","server":"cronjob","job":"trailingTradeIndicator","uuid":"b8a528ad-2651-492e-bf4f-4b7c7870bb75","level":50,"tag":"job-timeout","msg":"Failed to run the job within 20000ms.","time":"2022-08-11T15:00:13.013Z","v":0}

@habibalkhabbaz
Copy link
Contributor Author

habibalkhabbaz commented Aug 11, 2022

@habibalkhabbaz Is this because of racing the queue?


{"name":"binance-api","version":"0.0.88","hostname":"f545a1de3f6a","pid":46,"gitHash":"unspecified","server":"cronjob","job":"trailingTradeIndicator","uuid":"b8a528ad-2651-492e-bf4f-4b7c7870bb75","level":50,"tag":"job-timeout","msg":"Failed to run the job within 20000ms.","time":"2022-08-11T15:00:13.013Z","v":0}

Hello @uhliksk
I see it's happening to me too.
Let me check.
I think the problem coming from locking the symbol in trailingTrade
I am testing without locking now. If it's fine then I can solve it.

@habibalkhabbaz
Copy link
Contributor Author

habibalkhabbaz commented Aug 11, 2022

@uhliksk
Done. Pushed a fix.
Let me know if it's solved.

After implementing the queue, we don't need to do the trick of locking the symbol in trailingTrade.
The queue will do the job.

@chrisleekr let me know if you confirm me on this too.

@uhliksk
Copy link
Contributor

uhliksk commented Aug 11, 2022

@habibalkhabbaz I found it was actually redis-server consuming too many resources. Even after the fix applied I had to restart redis-server manually.

image

@habibalkhabbaz
Copy link
Contributor Author

habibalkhabbaz commented Aug 11, 2022

@uhliksk
Yes, there is a way to improve this. We have to re-use the Redis connections based on the documentation:

https://github.com/OptimalBits/bull/blob/master/PATTERNS.md#reusing-redis-connections

By doing this it will decrease the usage of Redis too much.

redis-usage

However, this will report the following to Node.js process

Possible EventEmitter memory leak detected. 11 error listeners added to [Commander]. Use emitter.setMaxListeners() to increase limit

This is not a real memory leak and we can ignore it using the following, which will make the listeners unlimited:

require('events').EventEmitter.defaultMaxListeners = 0;

Important note: using the above will make us sacrifice by the memory leak warnings because we may don't see any real memory leak warnings in future.

What do you think @uhliksk ?
I think @chrisleekr can help us on this too.

@uhliksk
Copy link
Contributor

uhliksk commented Aug 11, 2022

@habibalkhabbaz I didn't analyzed this yet but I see CPU usage of redis-server is raising again and it's consuming same %CPU even after binance-bot is restarted. To reduce %CPU back to almost zero I have to restart binance-redis itself. This looks like the issue is something persistent on redis-server. Isn't the database on redis-server just growing without removing the old records?

@uhliksk
Copy link
Contributor

uhliksk commented Aug 11, 2022

@habibalkhabbaz It is growing fast.

image

@habibalkhabbaz
Copy link
Contributor Author

@uhliksk
Ignore my last message.

Thanks for the info. It helps me to debug.
I found the issue. Going to push a fix.

@habibalkhabbaz
Copy link
Contributor Author

@uhliksk
Done.
Now the Redis CPU usage must be better and the keys will not increase horribly.

@uhliksk
Copy link
Contributor

uhliksk commented Aug 11, 2022

@habibalkhabbaz Great job! Thank you.

@habibalkhabbaz
Copy link
Contributor Author

habibalkhabbaz commented Aug 11, 2022

@uhliksk
If everything goes well, I think this PR will be ready to be reviewed by @chrisleekr 💪

Going to bed now. It's 2AM here haha.

@uhliksk
Copy link
Contributor

uhliksk commented Aug 12, 2022

Hello @habibalkhabbaz, there is no issue since last commit on my side. I think this PR is ready to be reviewed by @chrisleekr.

@chrisleekr chrisleekr self-requested a review August 12, 2022 13:09
@chrisleekr chrisleekr added the enhancement New feature or request label Aug 12, 2022
@chrisleekr
Copy link
Owner

@habibalkhabbaz @uhliksk

Hi guys,

It is great. :)

I will review/test it this weekend.

@chrisleekr
Copy link
Owner

chrisleekr commented Aug 12, 2022

Guys,

I've added Bull Board to the stack.
You can now access Bull Board /queue.

To be saving space, I limit it to keeping 100 completed tasks.
As the job is fast, you won't see much in the Active tab.

As this board does not have authentication, I think we can add in later on.
At this point, having a Bull board does not seem to give any benefit.
Let me know what you guys think.

So far the changes look good. I will continue to review/test.

image

@habibalkhabbaz
Copy link
Contributor Author

habibalkhabbaz commented Aug 12, 2022

Nice changes @chrisleekr 🎉

Update:

I see that the bull board is a good addition, and for the time being, we can use it for monitoring and debugging like we can see if a symbol received its ticks correctly or not and when.

Also, I think we can pass the job parameter from queue to executeTrailingTrade and use job.progress
Example:

  • Execute step 1
  • job.progress(10)
  • Execute step 2
  • job.progress(10)
  • Execute step 3
  • Etc ... till completing all the steps

Using this will show the progress of the job on the bull board and I think if the job failed we can see that the progress is not completed.
This is not really important but just an idea.

image

@chrisleekr
Copy link
Owner

@habibalkhabbaz

Yeah, that would be a good addition. 👍
Although, I think we can add that later on, not with this PR.
We can even save the result of the job to debug.

This queue implementation is pretty stable. I can see a lot of benefits.
Let's give one or two days more and see whether we can find any bug.

@chrisleekr
Copy link
Owner

I reviewed the code @habibalkhabbaz
Looks good. I think we are good to merge.
I will merge in tomorrow if there is no issue found.

@habibalkhabbaz
Copy link
Contributor Author

Thank you @chrisleekr
It seems good, there is no issue in my side till now.

@chrisleekr chrisleekr merged commit 36a3cf9 into chrisleekr:master Aug 14, 2022
@chrisleekr
Copy link
Owner

@habibalkhabbaz @uhliksk

It's merged. Thanks for your contributions guys.

I have a few things to fix before releasing a new version.
Once all is resolved, I will release it today.

@habibalkhabbaz habibalkhabbaz deleted the feat/introduce-queue branch August 16, 2022 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No indication of the buy order has been rejected on Binance
3 participants