-
Notifications
You must be signed in to change notification settings - Fork 4
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
initial draft of an introspection data model #9
base: master
Are you sure you want to change the base?
Conversation
For Project {Phantom}Drift.
phantom-drift/introspection.proto
Outdated
// streams within this connection by inlining. | ||
repeated Stream streams = 13; | ||
|
||
reserved 14, 15; |
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.
Could probably group some fields to spare more 1-byte ids.
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.
Well, we can change the EndpointPair
to Endpoint
and add transport
there. We can also create a new message Traffic
to handle the DataGauge
traffic_in
and traffic_out
on both Connection
and Stream
. What do you think?
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 like the Traffic idea! About the transport inside Endpoint, it sounds weird.
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 is shaping well raul! Great work!
I provided some feedback that I would like your input
phantom-drift/introspection.proto
Outdated
} | ||
|
||
// PeerId represents a peer ID. | ||
// TODO: At this point I'm unsure if we'll return the string or the bytes representation. |
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 think we can keep the id_bytes
for now. However, as we intend to provide the necessary data to the UI with no computations in the libp2p context, id_bytes
should not be necessary
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, I agree it should be a string only.
phantom-drift/introspection.proto
Outdated
// streams within this connection by inlining. | ||
repeated Stream streams = 13; | ||
|
||
reserved 14, 15; |
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.
Well, we can change the EndpointPair
to Endpoint
and add transport
there. We can also create a new message Traffic
to handle the DataGauge
traffic_in
and traffic_out
on both Connection
and Stream
. What do you think?
phantom-drift/introspection.proto
Outdated
// the status of this connection. | ||
Status status = 3; | ||
// the transport managing this connection. | ||
Transport transport = 4; |
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.
shouldn't this be just the transport name? I think the Transport
type has a lot of redundant information
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 changed this to a transport_id, and added a bytes id to Transport. How does that sound?
// connection listing. | ||
repeated Connection conns = 4; | ||
// streams listing. | ||
repeated Stream streams = 5; |
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.
We have the StreamList
in each Connection
. Shouldn't it be enough?
} | ||
|
||
// Host is a top-level object conveying a cross-cutting view of the entire system, including all subsystems. | ||
// TODO: WIP right now we're only covering the Swarm subsystem. |
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.
move this to the Subsystems
definition
Co-Authored-By: Vasco Santos <[email protected]>
@raulk FYI I added the DHT subsystem to the protobuf definition |
|
||
message Bucket { | ||
// bucket peers. | ||
repeated bytes peers = 1; |
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.
Things missing here:
- latency of each peer (we track this)
- average latency of the bucket
- distance of the bucket
- peer age in bucket
// latency for provide operations. | ||
repeated uint64 latency_provide_ns = 6; | ||
// counter for provide operations. | ||
ResultCounter operation_provide = 7; |
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.
typo, field duplicated.
repeated Bucket buckets = 2; | ||
} | ||
|
||
message Operations { |
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.
call this OperationStatistics
?
// type of the query. | ||
Type type = 5; | ||
// rpc message of the query. | ||
bytes rpc = 6; |
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.
Not sure this is needed.
} | ||
|
||
// id of the query. | ||
bytes id = 1; |
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.
ID of the target of the query, right?
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.
Since the type is bytes
and elsewhere peer Ids are string
, I suspect this is intended for internal referencing, like connection and stream ids? In which case, we should add a string
field like target_peer_id
?
(related: should the peer lists in Query and Bucket in the DHT section be repeated string
not repeated bytes
?)
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 ID can represent a peer ID or a CID. I'm fine making these strings for now, so that they're presentable on UI immediately. If we make them bytes, the consumer would be burdened with converting to a string prior to presenting.
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.
Note that this object can represent three types of queries: peer, provider, key/value.
For Project {Phantom}Drift (libp2p network visualizer). Some of these stats can be sourced from OpenCensus, others can't, and many elements are state, not stats.
Right now I'm only modelling the Swarm subsystem. Just getting some initial ideas across.
@vasco-santos – have a look.
This definition does not model the interaction between the tooling and the observed system. Right now it only tackles the data model. The protocol will be interactive and will likely make heavy use of gRPC messages and streams.