feat: add handshake protocol messages metrics

[mrekucci]

This change is 

[mrekucci]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @acud and @notanatol)

---

pkg/p2p/libp2p/internal/handshake/handshake.go, line 115 at r1 (raw file):

Previously, acud (acud) wrote…

exposing metrics from this package might be tricky... since the package is internal and metrics need to be registered inside the node.go file

Nice catch! Since the handshake is an internal part of the libp2p, I'll instantiate it as a part of the libp2p Matrics() call.

[acud]

I would suggest to split this into two: (1) metrics only PR (2) adding logging for specific debugging sessions which should not end up on master (and don't need a PR per-se)

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mrekucci and @notanatol)

---

pkg/p2p/libp2p/internal/handshake/handshake.go, line 253 at r2 (raw file):

	if err := r.ReadMsgWithContext(ctx, &syn); err != nil {
		s.metrics.SynRxFailed.Inc()
		s.logger.Tracef("handshake: read syn msg error; took: %s", time.Since(beginning))

you are logging and returning an error, which is not needed since the error would normally be logged anyway by the calling context. if timing info is needed you can instrument the error with it in the line below

---

pkg/p2p/libp2p/internal/handshake/handshake.go, line 257 at r2 (raw file):

	}
	s.metrics.SynRx.Inc()
	s.logger.Tracef("handshake: read syn msg took: %s", time.Since(beginning))

perhaps this can be added to an already existing logline, so we don't clutter the terminal with too much information? if specific debugging information is needed in order to find a bug this is fine, but then maybe we don't need to have it on master

---

pkg/p2p/libp2p/internal/handshake/handshake.go, line 299 at r2 (raw file):

	}); err != nil {
		s.metrics.SynAckTxFailed.Inc()
		s.logger.Tracef("handshake: write syn ack msg error; took: %s; total: %s", time.Since(start), time.Since(beginning))

same comment here about logging and returning an error. it is fine if it doesn't end up on master though)

[mrekucci]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @acud, @mrekucci, and @notanatol)

---

pkg/p2p/libp2p/internal/handshake/handshake.go, line 253 at r2 (raw file):

you are logging and returning an error

The trace logging is about knowing how much of time each step took in the protocol execution not about logging an error.

if timing info is needed you can instrument the error with it in the line below

It can be inferred but not with such granularity or when the step is successful.

I agree though that I can move the traces into a separate branch.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication