Add metrics for trade tracking. #574

debnil · 2020-11-03T19:22:41Z

This PR adds metrics to track trades. Thus far, it adds a handler interface and implementation. The metrics will be actually added after #508 is merged, because the architecture around adding individual metrics changes.

debnil · 2020-11-03T19:25:01Z

@nikhilsaraf WIP PR for trade metrics. The actual metrics are not added, since we change how metrics are added in the existing PRs, but all the functions to generate those metrics already exist - they just need to be called. Let me know if this handler architecture is what you had in mind when you suggested fillHandler and fillTracker as references.

nikhilsaraf

added comments to help with the general direction of this PR, thanks for the draft!

api/exchange.go

cmd/trade.go

plugins/tradeMetricsHandler.go

nikhilsaraf · 2020-11-05T21:14:09Z

plugins/tradeMetricsHandler.go

+}
+
+// TotalBaseVolume returns the total base volume.
+func (h *TradeMetricsHandler) TotalBaseVolume() (total float64) {


we should add additional metrics here -- maybe can return a map instead of individual metrics and call the function ComputeMetrics or something similar to indicate that we are (a) running calculations and (b) we will return the metrics result that we wanted to collect from this handler.

(Note: base units is model.Trade.Amount and quote units is model.Trade.Amount * model.Trade.Price)

totalBaseVolume (buy and sell action are both +ve) -- buying $100 and selling $100 should equal $200

totalQuoteVolume (buy and sell action are both +ve)

netBaseVolume (buy action is +ve, sell is -ve) -- buying $100 and selling $100 should equal $0

netQuoteVolume (buy action is -ve, sell action is +ve)

numTrades

avgTradeSizeBase (totalBaseVolume/numTrades)

avgTradeSizeQuote (totalQuoteVolume/numTrades)

avgTradePrice (simple average of the trade price)

vwap (volume-weighted average of the trade price) -- I think this simplifies to totalQuoteVolume/totalBaseVolume but you'll have to confirm

anything else you can think of that would be important to capture for trades?

PS: we will need to combine this with Reset() and make sure that we create a copy of the trades structure and then reset it, and then run our computations. This will ensure that there is a very small window in which we lose trades. We should also add a TODO NS at that point so we add proper locking around the trades field on the metrics handler, since we will face contention for it when writing from the HandleFills function which will be called from a separate thread.

I think this is everything (including that VWAP formula) -- super helpful, thank you!! I've made these changes.

support/metrics/metricsTracker.go

nikhilsaraf · 2020-11-05T21:29:21Z

support/metrics/metricsTracker.go

@@ -304,3 +311,41 @@ func (mt *MetricsTracker) sendEvent(eventType string, eventProps interface{}) er
 	}
 	return nil
 }
+
+// RegisterHandler adds an internal handler.
+func (mt *MetricsTracker) RegisterHandler(handler api.TradeMetricsHandler) {


we don't need any of these functions here.

instead inside sendUpdate we can call ComputeMetrics on the tradeMetricsHandler instance that we hold and merge that output with the update map that we are sending to sendEvent -- that's it!

debnil · 2020-11-06T00:23:52Z

@nikhilsaraf: awesome! I've done the bulk of this review, thanks so much. The only part that's left is the merging trade metrics with map. This only makes sense to do once the GUI metrics tracker is added in, since that changes the architecture of event updates.

nikhilsaraf

this is the right direction. I've added some inline comments and we can merge once completed.

we also need to add logic that will "start the trade metrics handler" process by registering it with the fill tracker.

you can take this PR out of draft stage and we can merge after the next set of changes.

plugins/tradeMetricsHandler.go

support/metrics/metricsTracker.go

nikhilsaraf · 2020-11-07T12:00:29Z

plugins/tradeMetricsHandler.go

-	return
+// HandleFill handles a new trade
+// Implements FillHandler interface
+func (h *TradeMetricsHandler) HandleFill(trade model.Trade) error {


we need to test this tradeMetricsHandler. For now let's have one simple test case, where we add 3 trades:

buy XLM/USDT

buy XLM/USDT

sell XLM/USDT

then we assert that the computed values on the struct returned from ComputeTradeMetrics is correct.

this TestComputeTradeMetrics would be very simple and would ensure our calculations are correct.

support/metrics/metricsTracker.go

debnil · 2020-11-20T03:06:37Z

@nikhilsaraf : most of the requested changes are made, but there's now an import cycle. plugins/tradeMetricsHandler imports models.Trade, which imports plugins indirectly. I don't think there's a way around the handler importing models.Trade, since that's the base data structure for all of its operations. Curious what you think the best fix is.

nikhilsaraf

can you point out where model.Trade imports plugins?

if there is a import cycle then the code won't compile.

nikhilsaraf · 2020-11-24T13:49:25Z

cmd/trade.go

@@ -921,6 +923,10 @@ func makeFillTracker(
 		}
 	}

+	metricsHandler := metricsTracker.GetHandler()


we should not pull the metricsHandler from within the metricsTracker but pass it directly to this function. i.e. we don't need the GetHandler() method on metricsTracker either.

this registration of the handler should happen before the if strategyFillHandlers != nil { paragraph. we want the strategy handlers to always be last because we execute the handler in the order in which they are added as well. I should have added this as a comment, can you please add this as well (that "handlers are executed in the order in which they are added")?

nikhilsaraf · 2020-11-24T13:49:45Z

plugins/metricsTracker.go

@@ -215,7 +216,7 @@ func MakeMetricsTrackerCli(
 		botStartTime:        botStartTime,
 		isDisabled:          isDisabled,
 		updateEventSentTime: nil,
-		handler:             plugins.MakeTradeMetricsHandler(),
+		handler:             handler,


nikhilsaraf · 2020-11-24T13:50:10Z

plugins/metricsTracker.go

-
-	return tradeMetrics
+// GetHandler returns the TradeMetricsHandler
+func (mt *MetricsTracker) GetHandler() *plugins.TradeMetricsHandler {


can remove this (as commented above)

nikhilsaraf · 2020-11-24T13:50:54Z

plugins/tradeMetricsHandler.go

+	trades := h.GetTrades()
+	h.Reset()
+
+	// Note that we don't consider fees right now, as we don't have the infrastructure to understand fee units when tracking trades.


nikhilsaraf · 2020-11-24T13:51:48Z

plugins/tradeMetricsHandler.go

+		quote := base * price
+
+		totalBaseVolume += base
+		totalPrice += price


don't think the math works out this way for price to get avgTradePrice accurately.

avgTradePrice should be totalQuoteVolume / totalBaseVolume (i.e. we don't need to track totalPrice) -- which we call vwap

not sure if I had suggested adding this, but if I had that is wrong.
we should split this up into avgBuyPrice and avgSellPrice
we already have vwap which is the price inclusive of both buys and sells so getting these separate buy/sell prices will be more informative

nikhilsaraf · 2020-11-24T13:54:03Z

plugins/tradeMetricsHandler.go

+			netQuoteVolume -= quote
+		} else {
+			netBaseVolume -= base
+			netQuoteVolume -= quote


may be a typo, should be netQuoteVolume += quote instead of netQuoteVolume -= quote

nikhilsaraf · 2020-11-24T13:55:04Z

plugins/tradeMetricsHandler.go

+	avgTradeSizeBase := totalBaseVolume / numTrades
+	avgTradeSizeQuote := totalQuoteVolume / numTrades
+	avgTradePrice := totalPrice / numTrades
+	avgTradeThroughputBase := totalBaseVolume / secondsSinceLastUpdate


can we rename this to tradeThroughputPerSecondBase (and quote equivalent)?

debnil added 4 commits October 30, 2020 16:23

Initial commit - create trade metrics handler plugin

51b70ac

Add handler and tracker interfaces and impls

2d78a35

Create and inject metrics handler.

92018eb

Refine API, hook in read trades.

7a113bb

Add metrics to common props.

69bc642

nikhilsaraf reviewed Nov 5, 2020

View reviewed changes

Address review - nikhilsaraf

40c5d20

Fix build issue.

79364e7

nikhilsaraf reviewed Nov 7, 2020

View reviewed changes

support/metrics/metricsTracker.go Outdated Show resolved Hide resolved

debnil added 2 commits November 19, 2020 17:44

Merge branch 'master' into num-trades-metrics

6876075

Address review - refactor handler and plugin relation.

ebdc97a

nikhilsaraf reviewed Nov 24, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics for trade tracking. #574

Add metrics for trade tracking. #574

debnil commented Nov 3, 2020

debnil commented Nov 3, 2020

nikhilsaraf left a comment

nikhilsaraf Nov 5, 2020

nikhilsaraf Nov 5, 2020

nikhilsaraf Nov 5, 2020

debnil Nov 6, 2020

nikhilsaraf Nov 5, 2020

debnil commented Nov 6, 2020

nikhilsaraf left a comment

nikhilsaraf Nov 7, 2020

debnil commented Nov 20, 2020

nikhilsaraf left a comment

nikhilsaraf Nov 24, 2020

nikhilsaraf Nov 24, 2020

nikhilsaraf Nov 24, 2020

nikhilsaraf Nov 24, 2020

nikhilsaraf Nov 24, 2020

nikhilsaraf Nov 24, 2020

nikhilsaraf Nov 24, 2020

Add metrics for trade tracking. #574

Are you sure you want to change the base?

Add metrics for trade tracking. #574

Conversation

debnil commented Nov 3, 2020

debnil commented Nov 3, 2020

nikhilsaraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debnil commented Nov 6, 2020

nikhilsaraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debnil commented Nov 20, 2020

nikhilsaraf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment