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

New API endpoints for managed keys #5357

Merged
merged 4 commits into from Jun 20, 2023
Merged

Conversation

sstanculeanu
Copy link
Contributor

@sstanculeanu sstanculeanu commented Jun 19, 2023

Reasoning behind the pull request

  • info about the managed keys should be exported while in multikey mode

Proposed changes

  • node/managed-keys/count for the total number of managed keys
  • node/managed-keys/eligible/:epoch for the eligible keys managed, in a specific epoch, from the same shard as the node
  • node/managed-keys/waiting/:epoch for the waiting keys managed, in a specific epoch, from the same shard as the node

Testing procedure

  • will be tested with feat branch

Response examples:

image
image

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

@sstanculeanu sstanculeanu self-assigned this Jun 19, 2023
Base automatically changed from managed_peers_monitor to feat/multikey_metrics June 20, 2023 11:09
@sstanculeanu sstanculeanu marked this pull request as ready for review June 20, 2023 11:21
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch coverage: 84.48% and project coverage change: +0.03 🎉

Comparison is base (31b2ab9) 80.27% compared to head (8fc476c) 80.31%.

❗ Current head 8fc476c differs from pull request most recent head 32f7133. Consider uploading reports for the commit 32f7133 to get more accurate results

Additional details and impacted files
@@                    Coverage Diff                    @@
##           feat/multikey_metrics    #5357      +/-   ##
=========================================================
+ Coverage                  80.27%   80.31%   +0.03%     
=========================================================
  Files                        697      697              
  Lines                      90246    90355     +109     
=========================================================
+ Hits                       72448    72570     +122     
+ Misses                     12624    12608      -16     
- Partials                    5174     5177       +3     
Impacted Files Coverage Δ
facade/initial/initialNodeFacade.go 95.00% <0.00%> (-3.71%) ⬇️
facade/nodeFacade.go 95.53% <0.00%> (-1.74%) ⬇️
factory/status/statusComponentsHandler.go 75.51% <75.00%> (-0.02%) ⬇️
factory/status/statusComponents.go 95.17% <88.23%> (-1.07%) ⬇️
node/external/nodeApiResolver.go 69.37% <90.00%> (+2.94%) ⬆️
api/groups/nodeGroup.go 100.00% <100.00%> (ø)
factory/api/apiResolverFactory.go 94.50% <100.00%> (+0.01%) ⬆️
keysManagement/managedPeersMonitor.go 100.00% <100.00%> (ø)
node/nodeRunner.go 76.56% <100.00%> (+3.24%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@iulianpascalau iulianpascalau self-requested a review June 20, 2023 11:53
return
}

keys, err := ng.getFacade().GetEligibleManagedKeys(epoch)
Copy link
Contributor

Choose a reason for hiding this comment

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

this works for older epochs but won't work for future epochs. Do you think it is relevant to keep the epoch parameter? I think the use case will be to check the current epoch status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, would make more sense.. updated

c.JSON(
http.StatusOK,
shared.GenericAPIResponse{
Data: gin.H{"keys": keys},
Copy link
Contributor

Choose a reason for hiding this comment

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

eligibleKeys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

c.JSON(
http.StatusOK,
shared.GenericAPIResponse{
Data: gin.H{"keys": keys},
Copy link
Contributor

Choose a reason for hiding this comment

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

waitingKeys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

}
return make([][]byte, 0), nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Missing comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@sstanculeanu sstanculeanu merged commit 561065c into feat/multikey_metrics Jun 20, 2023
4 checks passed
@sstanculeanu sstanculeanu deleted the new_endpoints branch June 20, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants