search

hyperledger-iroha / iroha

Iroha - A simple, enterprise-grade decentralized ledger
https://wiki.hyperledger.org/display/iroha
Apache License 2.0
443 stars 277 forks source link

feat: add /peers API endpoint to torii #5235

Closed mversic closed 1 week ago

mversic commented 2 weeks ago

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

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
0x009922 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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.