/operation_fee_stats endpoint #586

robertDurst · 2018-08-15T01:37:16Z

This a rough first draft for this endpoint. Below I have noted the things I am looking for feedback on.

Things that need improvement:

currently my query is not great - this performs three queries and gets a lot more data from horizon db than needed

Things up for discussion:

data to return -- currently returning:
- latest_ledger_base_fee: as per @stanford-scs
- mode: the most occurring, accepted transaction over the past five ledgers
- min: the smallest tx fee accepted transaction over the past five ledgers
hard coded limit - currently I hardcode the endpoint to get data from the last five ledgers. Is five a good number (based on how core/horizon deals with pending tx's)? Should this be an endpoint parameter?

The test schema build script generated a lot of files I have not included in this PR.

Closes: #516

bartekn · 2018-08-16T08:54:31Z

I need to check this with real data (I'm on mobile tethering so will do it once on a stable wifi) but doesn't a query like:

select
    min(fee_paid/operation_count),
    mode() within group (order by fee_paid/operation_count)
from history_transactions
where ledger_sequence > {current - x};

return the same data without doing it all manually?

Also because this won't change between ledgers we can keep the results in a cache for ~5 seconds so consecutive requests do not recalculate it over and over again.

robertDurst · 2018-08-16T19:29:47Z

As per discussion with @bartekn , next steps are:

Consolidating to only two queries (one for latest ledger info and one for x ledgers worth of tx's)
Moving calculation logic to SQL query
Implementing a cache for op_fee_stats modeled after https://github.com/stellar/go/blob/master/services/horizon/internal/ledger/main.go

Will update PR with this by Saturday night. Today and Friday I will be at RustConf

robertDurst · 2018-08-18T17:12:56Z

@bartekn switched to cache like we described. I made a few design decisions in this process:

Inserted op fee cache update into current horizon tick, I can easily make this every 5 ticks or create a separate tick that goes every 5 sec (my reasoning here was that ledgers too should only be updated every 5 sec, but the update lives within the 1 sec tick)
Hardcoded query to last 5 ledgers worth of tx's. This seems good for now (and also wasn't sure how a 5/10/15 approach would work, would the endpoint then return data for each of these queries). If you think this is something we need, this is also easy to add.

robertDurst · 2018-08-18T17:14:28Z

Also, tests do pass, but I think travis won't since I didn't add any of the generated test files beyond the scc recipes

bartekn · 2018-08-21T13:42:27Z

services/horizon/internal/actions_operation_fee_stats.go

+type OperationFeeStatsAction struct {
+	Action
+	Records     []history.Transaction
+	Ledgers     history.LedgerCache


Records and Ledgers not used.

bartekn · 2018-08-21T13:57:39Z

services/horizon/internal/app.go

+
+	err = a.HistoryQ().
+		TransactionsForLastXLedgers(latest.Sequence).
+		Select(&resp)


Can we move Select inside TransactionsForLastXLedgers and change it to something like:

TransactionsForLastXLedgers(currentSeq int32, min, max *null.Int) error

Then FeeStats is not needed anymore.

Unless I am mistaken (still not a golang pro yet), I believe I need some sort of intermediary data structure to capture the response from the sql query -- hence FeeStats and LatestLedger, both of which I moved to github.com/stellar/go/services/horizon/internal/db2/history.

I could accept and min and mode as input params and write to them as part of this method, however I would still need something like FeeStats right?

bartekn · 2018-08-21T13:59:51Z

services/horizon/internal/app.go

+
+	if len(resp) < 1 {
+		return
+	}


This query will always return at least 1 row.

bartekn · 2018-08-21T14:02:59Z

services/horizon/internal/app.go

+	type LatestLedger struct {
+		BaseFee  int32 `db:"base_fee"`
+		Sequence int32 `db:"sequence"`
+	}


Let's move this to github.com/stellar/go/services/horizon/internal/db2/history package.

bartekn · 2018-08-21T14:04:16Z

services/horizon/internal/db2/history/transaction.go

+			Select("min(fee_paid/operation_count),  mode() within group (order by fee_paid/operation_count)").
+			From("history_transactions").
+			Where("ledger_sequence > ?", currentSeq-5),
+	}


Why not GetRaw as in LatestLedgerBaseFeeAndSequence? This looks complicated.

bartekn · 2018-08-21T14:12:05Z

services/horizon/internal/app.go

+	err = a.HistoryQ().LatestLedgerBaseFeeAndSequence(&latest)
+	if err != nil {
+		goto Failed
+	}


Inserted op fee cache update into current horizon tick, I can easily make this every 5 ticks or create a separate tick that goes every 5 sec (my reasoning here was that ledgers too should only be updated every 5 sec, but the update lives within the 1 sec tick)

We can do it even better. We can leave the tick to be every second but then we can check if there is a new ledger (by comparing the result of LatestLedgerBaseFeeAndSequence and current state in operationfeestats). If there is a new ledger we can continue calculations, if not we can return early.

robertDurst · 2018-08-22T21:09:07Z

@bartekn updated for latest review. One part I have questions about (responded in the comment thread above) is where you said Then FeeStats is not needed anymore.

bartekn · 2018-08-23T16:34:58Z

services/horizon/internal/app.go

+	var next operationfeestats.State
+
+	var latest history.LatestLedger
+	var resp history.FeeStats


Can we rename resp to feeStats? Easy to mistake.

bartekn · 2018-08-23T16:42:57Z

services/horizon/internal/db2/history/transaction.go

@@ -16,6 +16,17 @@ func (q *Q) TransactionByHash(dest interface{}, hash string) error {
 	return q.Get(dest, sql)
 }

+// TransactionsForLastXLedgers filters the query to only the last X ledgers worth of transactions.
+// Currently, we hard code the query to return the last 5 ledgers worth of transactions. In the
+// future this mya be configurable.


mya -> may

bartekn · 2018-08-23T16:44:21Z

services/horizon/internal/operationfeestats/main.go

+func SetState(next State) {
+	lock.Lock()
+	current = next
+	lock.Unlock()


I think we need one more thing here to prevent sync issues (like in #603 but probably less dangerous). UpdateOperationFeeStatsState is called every second so if one of the queries inside take a long time, the old state can overwrite the newer one:

Time Tick1 Tick2

1 UpdateOperationFeeStatsState start

2 UpdateOperationFeeStatsState start

3 Save result: operationfeestats.SetState

4 Save result: operationfeestats.SetState

To prevent this we should:

Check if the next.LastLedger > current.LastLedger in SetState above.

Change TransactionsForLastXLedgers to something like TransactionFeesBetweenLedgers(first int32, last int32, dest interface{}). So we will always check the correct transactions even if one or more new ledgers appeared in a meantime.

bartekn

LGTM! Can you add test schema files?

bartekn · 2018-08-24T13:16:28Z

services/horizon/internal/test/t.go

+func (t *T) UpdateOperationFeeStatsState() {
+	var err error
+	var next operationfeestats.State
+	var latest history.LatestLedger


All Update* methods that are duplicated from App should be removed eventually. For now, let's just update them (ex. history db min/mode query is wrong).

robertDurst · 2018-08-24T18:27:43Z

Looks like a I have a couple things to fix -- some tests fail and a sort of cyclical import (no sure where that came from). Just rebased and will see if that fixes anything.

robertDurst · 2018-08-24T22:32:59Z

Looks like recent offer liabilities changes broke scc test scenarios

robertDurst · 2018-08-27T21:09:19Z

Tests pass, but needs one more round of cleanup/refactor.

robertDurst · 2018-08-27T21:30:48Z

@bartekn ok so test schema included and everything passes. Only questionable thing left I know of is a ResetState method I added to operationfeestats cache. I did this because the cache checks that new_ledger > cur_ledger. However since empty struct has 0 ledger seq as default, it cannot reset the cache.

robertDurst · 2018-09-10T17:11:02Z

Yeah 🍾

robertDurst requested review from bartekn, tomerweller and nikhilsaraf August 15, 2018 01:37

bartekn suggested changes Aug 21, 2018

View reviewed changes

bartekn reviewed Aug 23, 2018

View reviewed changes

bartekn reviewed Aug 24, 2018

View reviewed changes

bartekn mentioned this pull request Aug 24, 2018

Remove application ticking service #613

Open

robertDurst mentioned this pull request Aug 24, 2018

build_test_scenarios fails #614

Closed

operation_fee_stats endpoint

55fba30

bartekn approved these changes Aug 28, 2018

View reviewed changes

bartekn mentioned this pull request Aug 28, 2018

add operation fee statistics endpoint #516

Closed

nikhilsaraf mentioned this pull request Aug 29, 2018

cleanup horizon's root endpoint #239

Closed

bartekn added this to the Horizon v0.15.0 milestone Sep 4, 2018

bartekn changed the title ~~[WIP] /operation_fee_stats endpoint~~ /operation_fee_stats endpoint Sep 4, 2018

bartekn added the horizon label Sep 5, 2018

bartekn changed the base branch from master to horizon-0.15.0 September 10, 2018 16:38

Merge branch 'horizon-0.15.0' into fee-endpoint

d80fb46

bartekn merged commit 2e1fe77 into stellar:horizon-0.15.0 Sep 10, 2018

tomerweller mentioned this pull request Sep 10, 2018

manually enter operation fee, default to /operation_fee_stats stellar-deprecated/account-viewer#74

Open

bartekn pushed a commit that referenced this pull request Sep 24, 2018

operation_fee_stats endpoint (#586)

0b83f04

bartekn pushed a commit that referenced this pull request Sep 25, 2018

operation_fee_stats endpoint (#586)

55c6e89

bartekn pushed a commit that referenced this pull request Oct 10, 2018

operation_fee_stats endpoint (#586)

b6cd55d

bartekn pushed a commit that referenced this pull request Oct 25, 2018

operation_fee_stats endpoint (#586)

2b26549

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

/operation_fee_stats endpoint #586

/operation_fee_stats endpoint #586

robertDurst commented Aug 15, 2018 •

edited

Loading

bartekn commented Aug 16, 2018

robertDurst commented Aug 16, 2018 •

edited

Loading

robertDurst commented Aug 18, 2018

robertDurst commented Aug 18, 2018

bartekn Aug 21, 2018

bartekn Aug 21, 2018

robertDurst Aug 22, 2018

bartekn Aug 21, 2018

bartekn Aug 21, 2018

bartekn Aug 21, 2018

bartekn Aug 21, 2018

robertDurst Aug 22, 2018

robertDurst commented Aug 22, 2018

bartekn Aug 23, 2018 •

edited

Loading

bartekn Aug 23, 2018 •

edited

Loading

bartekn Aug 23, 2018 •

edited

Loading

bartekn left a comment

bartekn Aug 24, 2018

robertDurst commented Aug 24, 2018 •

edited

Loading

robertDurst commented Aug 24, 2018 •

edited

Loading

robertDurst commented Aug 27, 2018

robertDurst commented Aug 27, 2018

robertDurst commented Sep 10, 2018

Time	Tick1	Tick2
1	`UpdateOperationFeeStatsState` start
2		`UpdateOperationFeeStatsState` start
3		Save result: `operationfeestats.SetState`
4	Save result: `operationfeestats.SetState`

/operation_fee_stats endpoint #586

/operation_fee_stats endpoint #586

Conversation

robertDurst commented Aug 15, 2018 • edited Loading

bartekn commented Aug 16, 2018

robertDurst commented Aug 16, 2018 • edited Loading

robertDurst commented Aug 18, 2018

robertDurst commented Aug 18, 2018

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

robertDurst commented Aug 22, 2018

bartekn Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

bartekn Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

bartekn Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

bartekn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst commented Aug 24, 2018 • edited Loading

robertDurst commented Aug 24, 2018 • edited Loading

robertDurst commented Aug 27, 2018

robertDurst commented Aug 27, 2018

robertDurst commented Sep 10, 2018

robertDurst commented Aug 15, 2018 •

edited

Loading

robertDurst commented Aug 16, 2018 •

edited

Loading

bartekn Aug 23, 2018 •

edited

Loading

bartekn Aug 23, 2018 •

edited

Loading

bartekn Aug 23, 2018 •

edited

Loading

robertDurst commented Aug 24, 2018 •

edited

Loading

robertDurst commented Aug 24, 2018 •

edited

Loading