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

fix(corehttp): adjust peer counting metrics #8577

Merged
merged 1 commit into from
Dec 3, 2021

Conversation

schomatis
Copy link
Contributor

Following upstream fix in libp2p/go-libp2p-swarm@0538806.

Fixes #8135.

@schomatis schomatis requested a review from petar December 2, 2021 14:21
@schomatis schomatis self-assigned this Dec 2, 2021
Copy link
Contributor

@petar petar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

if actual["/ip4/tcp"] != float64(3) {
t.Fatalf("expected 3 peers, got %f", actual["/ip4/tcp"])
totalPeers := peersTransport["/ip4/tcp"] + peersTransport["/ip4/udp/quic"]
if totalPeers != 3 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is fine. However, I'll just note that it still depends on internals of libp2p. This test is creating "contact" to 3 peers, but it does not control what type of connections (and how many) are actually created to each peer (this is libp2p's business). Yet, this test checks for specific types of connections. So, if libp2p were to change and e.g. drop the quic transport, this test will fail again. In any event, I think the test is fine as it is, since it is unlikely libp2p will drop tcp or quic anytime soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I tried to retain the spirit of the test since this metric provides a map that collects transports, not just peer counts, but I'm also fine removing this level of specificity.

@petar petar merged commit 9d197ca into master Dec 3, 2021
@petar petar deleted the schomatis/fix/flaky-peers-total-test branch December 3, 2021 01:03
aschmahmann pushed a commit that referenced this pull request Dec 3, 2021
guseggert pushed a commit that referenced this pull request Dec 3, 2021
This was referenced Dec 8, 2021
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.

Flaky test: TestPeersTotal
2 participants