add itemBasedAnalyzer; refactor evm analyzers accordingly #511
Conversation
Andrew7234
force-pushed
the
andrew7234/item-based-analyzer
branch
from
9e1285b
to
bf95e40
Compare
Andrew7234
requested review from
aefhm,
mitjat,
pro-wh and
ptrus
as code owners
| } | ||
|
|
||
| func (p *processor) GetItems(ctx context.Context, limit uint64) ([]common.ChainName, error) { | ||
| return []common.ChainName{common.ChainNameMainnet}, nil |
There was a problem hiding this comment.
note: for this and the node_stats analyzer, this fixed queue means that these analyzers are not yet compatible with the e2e_regression tests. This will likely need to be slightly reworked withstopOnEmptyBatch but I'd prefer to leave it for a future PR as this one is already somewhat long, and the regression tests do not currently test those analyzers so there is no loss of functionality.
There was a problem hiding this comment.
does this fully coincide with the use of fixedInterval?
There was a problem hiding this comment.
👍 I'd say at least for now, let's just exclude those two analyzers from e2e tests because they don't interact with other analyzers in a very meaningful way. We can include them later perhaps, though I'm a little doubtful it's even worth the hassle.
There was a problem hiding this comment.
does this fully coincide with the use of fixedInterval?
currently yes
There was a problem hiding this comment.
lotsa red! 🤘
and with tests ❤️
Looks good by and large, I have only one or two nontrivial comments.
Despite the new unit tests, can you please also run e2e tests? It's a pretty big systemic change.
When running locally, you don't need to worry about caches and/or analyzers supporting StopOnEmptyBatch ... just enable all analyzers in config, then manually run analysis, kill analyzers when you can tell from the logs they're done, and run run.sh (which you possibly want to extend with an endpoint or two to exercise all the analyzers).
analyzer/item/item.go
Outdated
| return | ||
| } | ||
| // Update queueLength | ||
| a.sendQueueLengthMetric(ctx) |
There was a problem hiding this comment.
I'm a little worried that the SQL query that determines the queue length might end up being the most expensive operation of them all, which is hard to justify.
No action needed, just something to keep an eye on. We can make this more complicated and opt to only update the metric if it's older than a minute, or similar.
| } | ||
|
|
||
| func (p *processor) GetItems(ctx context.Context, limit uint64) ([]common.ChainName, error) { | ||
| return []common.ChainName{common.ChainNameMainnet}, nil |
There was a problem hiding this comment.
👍 I'd say at least for now, let's just exclude those two analyzers from e2e tests because they don't interact with other analyzers in a very meaningful way. We can include them later perhaps, though I'm a little doubtful it's even worth the hassle.
Andrew7234
force-pushed
the
andrew7234/item-based-analyzer
branch
from
c0986a3
to
8793158
Compare
Andrew7234
force-pushed
the
andrew7234/item-based-analyzer
branch
from
9bbe343
to
d2424ed
Compare
enable stopOnEmptyBatch nit: pointer receivers refactor node_stats and metadata_registry analyzers move metadata_registry to own package add itemBasedAnalyzer tests adjust tests nit nits test nits move closingChannel() to util address comments add interItemDelay flag refactor item analyzer args into cfg change stopOnEmptyBatch -> stopOnEmptyQueue propagate error from batch; nit linter update tests nit
Andrew7234
force-pushed
the
andrew7234/item-based-analyzer
branch
from
fffaac2
to
15c5add
Compare
https://app.clickup.com/t/866afuw9w
Reviewers:
item.gois the main file of interest, please start there :)Design spec:
https://docs.google.com/document/d/11JiEg5-FYXMxWEvizsXN7JdC3mTYS_nfKEm1_BxyaFA/edit#heading=h.fe8yeyiyi2yc
Todo:
Testing:
Added endpoints to the e2e tests to test the item based analyzers and compared the results between
mainand this branch. Results were out of order (since no order is specified for some endpoints) but identical as expected.