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

feat: add /peers API endpoint to torii #5235

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

mversic
Copy link
Contributor

@mversic mversic commented Nov 14, 2024

Context

Ever since #5117 clients have lost the ability to find about other nodes in the network except for the node they are connecting to. This is because FindPeers doesn't return address anymore, just peers's public key. We can't return data that is not on chain in this query so I've implemented another torii endpoint

Solution

  • add new torii endpoint /peers

Alternatives

I see that we have /status and /metrics. I'm not sure why we have both since, from what I can see, /status is a superset of `/metrics. Considering this, I can make it so that peers are returned on one of those endpoints instead of adding a new point. I implemented this functionality on a separate endpoint because:

  1. there can be lots of peers returned and we may want to batch this response
  2. we don't want the client to pay the price of retrieving this long list of peers if they are interested in simple metrics

@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Nov 14, 2024
s8sato
s8sato previously approved these changes Nov 14, 2024
crates/iroha_core/src/state.rs Show resolved Hide resolved
@0x009922
Copy link
Contributor

I see that we have /status and /metrics. I'm not sure why we have both since, from what I can see, /status is a superset of `/metrics. Considering this, I can make it so that peers are returned on one of those endpoints instead of adding a new point. I implemented this functionality on a separate endpoint because:

1. there can be lots of peers returned and we may want to batch this response

2. we don't want the client to pay the price of retrieving this long list of peers if they are interested in simple metric

Well, in fact, the /status endpoint already supports selective retrieval (see sub-routing in the reference), i.e. GET /status/peers "just works" already. I would say introducing a new route is not as nice as reusing the current one.

What do you think?

@mversic
Copy link
Contributor Author

mversic commented Nov 15, 2024

I see that we have /status and /metrics. I'm not sure why we have both since, from what I can see, /status is a superset of `/metrics. Considering this, I can make it so that peers are returned on one of those endpoints instead of adding a new point. I implemented this functionality on a separate endpoint because:

1. there can be lots of peers returned and we may want to batch this response

2. we don't want the client to pay the price of retrieving this long list of peers if they are interested in simple metric

Well, in fact, the /status endpoint already supports selective retrieval (see sub-routing in the reference), i.e. GET /status/peers "just works" already. I would say introducing a new route is not as nice as reusing the current one.

What do you think?

I noticed this. While I agree that introducing a new endpoint is not as nice, my thinking was that /status should have a short response so I didn't want to add peers there which could be a long list with a few thousand entries there. Maybe you can give some more thoughts on it?

@0x009922
Copy link
Contributor

I noticed this. While I agree that introducing a new endpoint is not as nice, my thinking was that /status should have a short response so I didn't want to add peers there which could be a long list with a few thousand entries there. Maybe you can give some more thoughts on it?

Mmm, I think we could advise users to use sub-routing at all times. If they don't, well, they'll get a large piece of JSON. Still, even thousands of pubkey strings isn't that enormous, and happens in big production environments. If someone works with a really huge network, they would be probably knowledgeable of sub-routing feature.

In addition, it could be useful to not just add { peers: PeerId[] } field, but add a count to it: { peers: { count: 1234, list: [ ... ] } }, accessible by /status/peers/count. That could be useful for those uninterested in the exact identifiers, but only in the count of them.

@mversic
Copy link
Contributor Author

mversic commented Nov 15, 2024

I noticed this. While I agree that introducing a new endpoint is not as nice, my thinking was that /status should have a short response so I didn't want to add peers there which could be a long list with a few thousand entries there. Maybe you can give some more thoughts on it?

Mmm, I think we could advise users to use sub-routing at all times. If they don't, well, they'll get a large piece of JSON. Still, even thousands of pubkey strings isn't that enormous, and happens in big production environments. If someone works with a really huge network, they would be probably knowledgeable of sub-routing feature.

In addition, it could be useful to not just add { peers: PeerId[] } field, but add a count to it: { peers: { count: 1234, list: [ ... ] } }, accessible by /status/peers/count. That could be useful for those uninterested in the exact identifiers, but only in the count of them.

May I then suggest that /status keeps returning brief info as before. And /status/peers returns full info?

@DCNick3
Copy link
Contributor

DCNick3 commented Nov 15, 2024

Code-wise - looking good.

Design-wise - also not a fan of additional endpoints.

May I then suggest that /status keeps returning brief info as before. And /status/peers returns full info?

How about /status?include=peers? This would leave a door open for adding other potentially large fields.

@DCNick3 DCNick3 self-assigned this Nov 18, 2024
@mversic mversic requested a review from s8sato November 19, 2024 06:47
@mversic mversic merged commit a8cf1c1 into hyperledger-iroha:main Nov 19, 2024
14 checks passed
@mversic mversic deleted the peers_endpoint branch November 19, 2024 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants