Skip to content
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

Add ForkDigest Filter #206

Merged

Conversation

spacesailor24
Copy link
Contributor

@spacesailor24 spacesailor24 commented Sep 22, 2022

Adds peerFilter:

input PeerFilter {
  forkDigest: String
}

to the following GraphQL APIs:

  • aggregateByAgentName(peerFilter: PeerFilter)
  • aggregateByCountry(peerFilter: PeerFilter)
  • aggregateByOperatingSystem(peerFilter: PeerFilter)
  • aggregateByNetwork(peerFilter: PeerFilter)
  • aggregateByHardforkSchedule(peerFilter: PeerFilter)
  • aggregateByClientVersion(peerFilter: PeerFilter)
  • getHeatmapData(peerFilter: PeerFilter)
  • getNodeStats(peerFilter: PeerFilter)
  • getNodeStatsOverTime(start: Float!, end: Float!, peerFilter: PeerFilter)
  • getRegionalStats(peerFilter: PeerFilter)
  • getAltairUpgradePercentage(peerFilter: PeerFilter)

Example Usage

query FilterByForkDigest {
  aggregateByHardforkSchedule(peerFilter: { forkDigest: "0x7d3b7f73" }) {
    	count,
    	version
  }
}

closes #205

Comment on lines +69 to 106
func (s mongoStore) GetHistory(ctx context.Context, start int64, end int64, peerFilter *model.PeerFilter) ([]*models.HistoryCount, error) {
var filter primitive.D
if peerFilter != nil &&
peerFilter.ForkDigest != nil {
forkDigest := *(peerFilter.ForkDigest)
if forkDigest[0:2] == "0x" {
forkDigest = forkDigest[2:]
}
forkDigestBytes, err := hex.DecodeString(forkDigest)
if err != nil {
return nil, err
}
filter = bson.D{
{
Key: "$match", Value: bson.D{{Key: "fork_digest", Value: forkDigestBytes}},
},
{
Key: "$and", Value: bson.A{
bson.D{{Key: "time", Value: bson.D{{Key: "$gt", Value: start}}}},
bson.D{{Key: "time", Value: bson.D{{Key: "$lt", Value: end}}}},
},
},
}
if err != nil {
return nil, err
}
} else {
filter = bson.D{
{
Key: "$and", Value: bson.A{
bson.D{{Key: "time", Value: bson.D{{Key: "$gt", Value: start}}}},
bson.D{{Key: "time", Value: bson.D{{Key: "$lt", Value: end}}}},
},
},
}
}

cursor, err := s.coll.Find(ctx, filter)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if there's a better way to write this given that the Find on line 106
(cursor, err := s.coll.Find(ctx, filter)) expects filter to be of type bson.D

aggregateByClientVersion(peerFilter: PeerFilter): [ClientVersionAggregation!]!
getHeatmapData(peerFilter: PeerFilter): [HeatmapData!]!
getNodeStats(peerFilter: PeerFilter): NodeStats!
getNodeStatsOverTime(start: Float!, end: Float!, peerFilter: PeerFilter): [NodeStatsOverTime!]!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested all the other methods using peerFilter locally, but getNodeStatsOverTime always returned:

{
  "data": {
    "getNodeStatsOverTime": []
  }
}

before and after the peerFilter additions, so I'm not 100% sure this is working as expected

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking

Copy link
Member

@sadiq1971 sadiq1971 Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spacesailor24, That's totally fine. getNodeStatsOverTime returns the count of nodes each day from the History table. A scheduler pushes data to the history table each day. So, ideally, you won't see anything with that query unless you run the crawler for at least a day. Change this to second and you will see data.

_, err = scheduler.AddFunc("@daily", c.insertToHistory)

Also, I have tested the APIs. Working fine with or without peerFilter.
So, with this implementation, if no filter is provided it returns all data.
That looks good to me. UI can filter based on the mainnet.

@spacesailor24 spacesailor24 marked this pull request as ready for review October 8, 2022 05:07
}

var err error
if peerFilter != nil &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add the nil checking inside AddForkDigestFilterToQueryPipeline so we don't need to check it every places.

Copy link
Contributor Author

@spacesailor24 spacesailor24 Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sadiq1971 Should AddForkDigestFilterToQueryPipeline error if peerFilter or peerFilter.forkDigest is nil?
Instead, I could refactor the method to AddPeerFilterToQueryPipeline and it would return an unmodified pipeline if peerFilter or peerFilter.forkDigest was nil or empty

This commit does the above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that exactly what I said. Will just add to pipeline if not nil

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge it.

@sadiq1971 sadiq1971 requested a review from philknows October 13, 2022 15:53
@spacesailor24 spacesailor24 merged commit 55bf484 into ChainSafe:main Oct 14, 2022
@spacesailor24 spacesailor24 deleted the wyatt/205-fork-digest-filter branch October 14, 2022 02:19
@sadiq1971 sadiq1971 mentioned this pull request Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ForkDigest filter
3 participants