-
Notifications
You must be signed in to change notification settings - Fork 0
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: aggregate metrics #41
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
DROP TABLE IF EXISTS indexer_aggregates; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
CREATE TABLE IF NOT EXISTS indexer_aggregates | ||
( | ||
id SERIAL PRIMARY KEY, | ||
timestamp BIGINT NOT NULL, | ||
graph_account VARCHAR(255) NOT NULL, | ||
message_count BIGINT NOT NULL, | ||
subgraphs_count BIGINT NOT NULL, | ||
UNIQUE(graph_account, timestamp) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also interested in additional summary like total topics covered, average message count (or some other things mentioned in the message propagation issues), but yeah some are not so easy to get from the existing code
It looks like the two HashMaps are indexed by indexer address, so having a Vec for active_indexers doesn't seem so useful when they can grab the keys from one of the maps.
I also think it might be easier to simply return the vec of IndexerStats (same thing as
AggregatedIndexerStats
?) so that users can do manipulation independently with greater flexibilityThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean all distinct ipfs hashes that Listener Radio has received messages for?
i don't quite get this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
you have the total_message_count for each indexer already, I'm interested in the average of that across all indexers. but, as I alluded to in the previous comment, it is something the client can do once they receive the vec of indexerStats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree, let's leave it to the client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will add one for distinct ipfs hashes thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, thanks.
What do you think about returning
Vec<IndexerStats>
directly as part ofSummary
instead of having to calculatetotal_message_count
andaverage_subgraph_account
? I think that allows more flexibility on the client side like calculating averages; but I can see an argument of providing that functionality to the client out-of-the-box... a bit of a tricky balance