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

To add bandwidth metrics #35

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

chiangmaioneluv
Copy link

To add basic bandwidth metrics + update protobuffs for communicating with hivemind

  1. uses default bandwidth counter from libp2p
  2. can be turned on / off on daemon start up with bandwidthMetricsEnabled option
  3. nice handle all the way to the DHT class in hivemind

pb/Makefile Outdated
@@ -4,3 +4,11 @@ all: $(pbgos)

%.pb.go: %.proto
protoc --gogofast_out=. --proto_path=$(GOPATH)/src:. $<

Copy link
Member

Choose a reason for hiding this comment

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

Please resolve the merge conflict and make sure it works

optional double selfRateIn = 1;
optional double selfRateOut = 2;
repeated PeerInfo peers = 3;
}
Copy link
Member

@justheuristic justheuristic Nov 28, 2023

Choose a reason for hiding this comment

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

missing end-of-file /n

@justheuristic
Copy link
Member

@dvmazur please take a look - you're way better at go than I am)

Copy link
Member

@justheuristic justheuristic left a comment

Choose a reason for hiding this comment

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

My minor comments were resolved. Pending review from @dvmazur

@dvmazur
Copy link
Collaborator

dvmazur commented Nov 28, 2023

Hey, will take a look at this PR by tomorrow evening.

d.bandwidth_metrics = metrics.NewBandwidthCounter()
opts = append(opts, libp2p.BandwidthReporter(d.bandwidth_metrics))
} else {
d.bandwidth_metrics = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: d.bandwidth_metrics are nil be default anyway.
ignore this comment if you wish

@dvmazur
Copy link
Collaborator

dvmazur commented Nov 29, 2023

Other than the optional change I've suggested above, I have nothing to add to @justheuristic's comments.

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.

4 participants