-
Notifications
You must be signed in to change notification settings - Fork 9
[explorer/nodewatch]: statistics service migration #1421
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
base: dev
Are you sure you want to change the base?
Conversation
… key Problem: The endpoint is missing when using the main public key to query node info. Solution: Added a new endpoint to query node info by the main public key. Added error handler for 400 and 404
… key Problem: The endpoint is missing when using the node public key to query node info. Solution: A new endpoint was added to query node info using the node public key.
Problem: The API node status field is missing Solution: Added API node status properties in NodeDescriptor
Problem: The filter node is missing on the API node list endpoint Solution: Added query params only_ssl, limit, and order only_ssl: to filter out SSL enable node, it needs explorer and wallet limit: the number of numbers returned from the endpoint order: shuffle node list when require
Problem: missing geo-location information in the node list Solution: load the geo location file from the specific path, and create a mapping for node
Problem: The nodes count endpoint is missing. Solution: Read the JSON file `symbol_time_series_nodes_count` and allow query data from the new endpoint.
Problem: missing finalizedEpoch, finalizedHash, and finalizedPoint in node descriptor Solution: added finalized properties in node descriptor
fb779b2
to
0842cb0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1421 +/- ##
=======================================
Coverage 98.26% 98.27%
=======================================
Files 163 158 -5
Lines 6691 6648 -43
Branches 143 143
=======================================
- Hits 6575 6533 -42
+ Misses 116 115 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
@@ -23,6 +23,13 @@ def __init__( | |||
height=0, | |||
finalized_height=0, | |||
balance=0, | |||
finalized_epoch=0, | |||
finalized_hash=None, | |||
finalized_point=0, |
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.
Does it make sense to group all the finalized properties into a namedtuple?
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.
Yes, sound good to me 👍🏼
I'll update it.
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.
An update.
I'm unsure it's a good practice to use nested way to get attr in Python.
Used namedtuple to group finalized_height
, finalized_epoch
, finalized_hash
, and finalized_point
as finalized_info.
Found out that chart_utlis.py's VersionAggregator
consumes finalized_height directly from the object instead of finalized_info.height
. Because of the namedtuple, it required a nested approach to retrieve the attribute.
Here is my commit d5f6ed7
# Act: | ||
json_object = repository.node_descriptors[3].to_json() | ||
|
||
print(json_object) |
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.
Is the print still needed?
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.
Good catch! 👍🏼
self.assertEqual(None, json_object['geoLocation']) | ||
|
||
# endregion | ||
|
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.
should be 2 spaces between # endregion
and the next.
full_node_names = list(map(lambda descriptor: descriptor['name'], all_node_descriptors)) | ||
random_node_names = list(map(lambda descriptor: descriptor['name'], node_descriptors)) | ||
|
||
self.assertEqual(2, len(node_descriptors)) |
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.
might also want to verify that all_node_descriptors
is not 2 nodes
for name in random_node_names: | ||
self.assertIn(name, full_node_names) | ||
|
||
def test_can_generate_nodes_json_filtered_limit_5(self): |
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.
This seems similar to the above? Not sure if checking the roles below matters for a limit test.
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.
Good catch! I was missing order='random'
params in the test.
] == list(map(lambda descriptor: descriptor['name'], response_json)) | ||
|
||
def test_get_api_symbol_nodes_peer_with_only_ssl(client): # pylint: disable=redefined-outer-name | ||
_assert_get_api_nodes(client.get('/api/symbol/nodes/peer?only_ssl'), ['ibone74']) |
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 thought only_ssl
flag would only work wtih API endpoint /api/symbol/nodes/api
? 🤔
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.
Nope. It belongs to both endpoints.
/api/symbol/nodes/api
: returns only roles=2
. For other roles, such as roles = 3
, it also includes only_ssl
.
Problem: missing some of the features and endpoints to migrate from the statistic service to Node Watch.
Solution: Added those endpoints and features.