aleem-ahmed
commented
3 years ago the last part
"When that fails, it will only log. What does that mean for your application? It gets logged, but now there is a hole in the data. Like I mentioned in an earlier issue, better error handling is needed. This should almost certainly be a "code red" for your application. If the data gets out of sync, everything suffers. If it suffers silently, that could be catastrophic."
is it really possible for the code to fail? and if so how would i handle the error? before i suggest a solution you should know that the neat think about these trades is that they have sequence numbers, so maybe I could make the server run some kind of check to see if all trades are existent by checking the sequence numbers. if anything is missing i can get the trade by querying by the sequence number to the api to fill in the "holes"
It makes sense to cache the results of the CBPro API and vend them yourself. I also could see why you chose to use a database to store that, but there's a few things related to this that you need to know. I'll list them below, but I wonder if it's even necessary to go through them. If the server can just call the CBPro API, and maintain it's history that way, why do we need to store it ourselves? Does it really provide any value? If it can all fit into memory, and that would make the app faster, why not just call CBPro and cache the results?
In general, some notes about db usage:
Think about how many reads/writes this database can support. Theres a bunch of places your making calls that could potentially get out of hand over time: a. https://github.com/aleem-ahmed/eccentric-trader/blob/0691dce6c969561bb316a21b45a4296e3b030437/server.js#L73 This one will save a document to the database on every tick from the websocket. That's already a lot of writes to the database and it scales as the load from the websocket increases, which could be an issue. b. https://github.com/aleem-ahmed/eccentric-trader/blob/0691dce6c969561bb316a21b45a4296e3b030437/client/collections/TickersCollection.js#L27 These functions could just use some love. readAllAll is not a good name. It also does basically the same thing as readAllWithinTimeFrame, so you can DRY it up. BTW 'readAllWitinTimeFrame' is spelled wrong: https://github.com/aleem-ahmed/eccentric-trader/blob/0691dce6c969561bb316a21b45a4296e3b030437/client/collections/TickersCollection.js#L38 One more note, you almost never want to select * from a database, which I think is what readAllAll currently does. What if this database grows into GBs or even bigger? do you really want to return everything?
Consistency a. Think about this line- https://github.com/aleem-ahmed/eccentric-trader/blob/0691dce6c969561bb316a21b45a4296e3b030437/server.js#L45 It looks like you are executing some code on every response from the webhook. In that block, you save it to the database. https://github.com/aleem-ahmed/eccentric-trader/blob/0691dce6c969561bb316a21b45a4296e3b030437/server.js#L73-L74 When that fails, it will only log. What does that mean for your application? It gets logged, but now there is a hole in the data. Like I mentioned in an earlier issue, better error handling is needed. This should almost certainly be a "code red" for your application. If the data gets out of sync, everything suffers. If it suffers silently, that could be catastrophic. b. I called this blurb consistency because while thinking about the above I want you to reference the CAP theorem. In the case of this app, which two guarantees of the three in the CAP theorem would you want to provide?