Skip to content

p2p: change metered connection ip and node id#19007

Closed
kurkomisi wants to merge 3 commits intoethereum:masterfrom
kurkomisi:p2p-metrics-node-id
Closed

p2p: change metered connection ip and node id#19007
kurkomisi wants to merge 3 commits intoethereum:masterfrom
kurkomisi:p2p-metrics-node-id

Conversation

@kurkomisi
Copy link
Copy Markdown
Contributor

@kurkomisi kurkomisi commented Feb 6, 2019

This PR changes two fields of the metered connection in the p2p package:

  • IP -> Addr in order to include the port
  • ID -> Node in order to use the node URL instead of the pure node ID

The corresponding MeteredPeerEvent fields' types are string in order to prevent the unwanted modification of the original objects.
This can be changed using deepcopy if needed.

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Feb 19, 2019

@kurkomisi I don't understand why the change to Node is needed. Connections can be identified by ID and IP/port. Why do you need the node URL?

@kurkomisi
Copy link
Copy Markdown
Contributor Author

kurkomisi commented Feb 19, 2019

@fjl you are right. When I created this PR I needed the node here for other metering purposes too, but I could resolve that more elegantly, so it might not make sense anymore.

The reason why I think this change can be still useful is that otherwise on the dashboard I can't show the Scheme, the RawQuery and the Fragment of the peer URL.
The String method of the node however returns the URL correctly, and also I can use the ID field of the node without further string manipulation.

@kurkomisi
Copy link
Copy Markdown
Contributor Author

Now the node id change is reverted, and instead I introduced a new event, which is triggered after the protocol handshake, where the peer information can entirely be extracted. I think we need this, because we want to send most of the data to the dashboard.

It isn't sure whether the encryption handshake event is still needed, but maybe we want to visualize the peers that manage to make the encHandshake, and fail before/during the protoHandshake, so I would keep it for the time being.

@kurkomisi
Copy link
Copy Markdown
Contributor Author

Superseded by #19397

@kurkomisi kurkomisi closed this Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants