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

Import go-log directly (except in GPBFT) and remove Logging #427

Merged
merged 5 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions certexchange/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
"runtime/debug"
"time"

"github.com/filecoin-project/go-f3"
"github.com/filecoin-project/go-f3/certs"
"github.com/filecoin-project/go-f3/gpbft"
"github.com/libp2p/go-libp2p/core/host"
Expand All @@ -27,8 +26,6 @@
Host host.Host
NetworkName gpbft.NetworkName
RequestTimeout time.Duration

Log f3.Logger
}

func (c *Client) withDeadline(ctx context.Context) (context.Context, context.CancelFunc) {
Expand All @@ -44,7 +41,7 @@
defer func() {
if perr := recover(); perr != nil {
_err = fmt.Errorf("panicked requesting certificates from peer %s: %v\n%s", p, perr, string(debug.Stack()))
c.Log.Error(_err)
log.Error(_err)

Check warning on line 44 in certexchange/client.go

View check run for this annotation

Codecov / codecov/patch

certexchange/client.go#L44

Added line #L44 was not covered by tests
}
}()

Expand Down Expand Up @@ -75,7 +72,7 @@
bw := bufio.NewWriter(stream)

if err := req.MarshalCBOR(bw); err != nil {
c.Log.Debugf("failed to marshal certificate exchange request to peer %s: %w", p, err)
log.Debugf("failed to marshal certificate exchange request to peer %s: %w", p, err)

Check warning on line 75 in certexchange/client.go

View check run for this annotation

Codecov / codecov/patch

certexchange/client.go#L75

Added line #L75 was not covered by tests
return nil, nil, err
}
if err := bw.Flush(); err != nil {
Expand All @@ -91,7 +88,7 @@
}
err = resp.UnmarshalCBOR(br)
if err != nil {
c.Log.Debugf("failed to unmarshal certificate exchange response header from peer %s: %w", p, err)
log.Debugf("failed to unmarshal certificate exchange response header from peer %s: %w", p, err)
return nil, nil, err
}

Expand All @@ -105,7 +102,7 @@
go func() {
defer func() {
if perr := recover(); perr != nil {
c.Log.Errorf("panicked while receiving certificates from peer %s: %v\n%s", p, perr, string(debug.Stack()))
log.Errorf("panicked while receiving certificates from peer %s: %v\n%s", p, perr, string(debug.Stack()))

Check warning on line 105 in certexchange/client.go

View check run for this annotation

Codecov / codecov/patch

certexchange/client.go#L105

Added line #L105 was not covered by tests
}
cancelReq()
close(ch)
Expand All @@ -122,12 +119,12 @@
case io.EOF:
return
default:
c.Log.Debugf("failed to unmarshal certificate from peer %s: %w", p, err)
log.Debugf("failed to unmarshal certificate from peer %s: %w", p, err)
return
}
// One quick sanity check. The rest will be validated by the caller.
if cert.GPBFTInstance != request.FirstInstance+i {
c.Log.Warnf("received out-of-order certificate from peer %s", p)
log.Warnf("received out-of-order certificate from peer %s", p)

Check warning on line 127 in certexchange/client.go

View check run for this annotation

Codecov / codecov/patch

certexchange/client.go#L127

Added line #L127 was not covered by tests
return
}

Expand Down
2 changes: 2 additions & 0 deletions certexchange/polling/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package polling

import (
"github.com/benbjohnson/clock"
logging "github.com/ipfs/go-log/v2"
)

var log = logging.Logger("f3/certexchange")
var clk clock.Clock = clock.New()
4 changes: 0 additions & 4 deletions certexchange/polling/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,12 @@ import (
"github.com/filecoin-project/go-f3/sim/signing"

"github.com/benbjohnson/clock"
logging "github.com/ipfs/go-log/v2"
"github.com/stretchr/testify/require"
)

// The network name used in tests.
const TestNetworkName gpbft.NetworkName = "testnet"

// The logger used in tests.
var TestLog = logging.Logger("f3-testing")

// The clock used in tests. Time doesn't pass in tests unless you add time to this clock.
var MockClock = clock.NewMock()

Expand Down
2 changes: 0 additions & 2 deletions certexchange/polling/poller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ func TestPoller(t *testing.T) {
NetworkName: polling.TestNetworkName,
Host: serverHost,
Store: serverCs,
Log: polling.TestLog,
}

require.NoError(t, server.Start())
Expand All @@ -62,7 +61,6 @@ func TestPoller(t *testing.T) {
client := certexchange.Client{
Host: clientHost,
NetworkName: polling.TestNetworkName,
Log: polling.TestLog,
}

poller, err := polling.NewPoller(ctx, &client, clientCs, backend)
Expand Down
8 changes: 4 additions & 4 deletions certexchange/polling/subscriber.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
}()

if err := s.run(ctx); err != nil && ctx.Err() == nil {
s.Log.Errorf("polling certificate exchange subscriber exited early: %s", err)
log.Errorf("polling certificate exchange subscriber exited early: %s", err)

Check warning on line 67 in certexchange/polling/subscriber.go

View check run for this annotation

Codecov / codecov/patch

certexchange/polling/subscriber.go#L67

Added line #L67 was not covered by tests
}
}()

Expand Down Expand Up @@ -115,7 +115,7 @@
nextInterval := predictor.update(progress)
nextPollTime := pollTime.Add(nextInterval)
delay := max(clk.Until(nextPollTime), 0)
s.Log.Infof("predicted interval is %s (waiting %s)", nextInterval, delay)
log.Infof("predicted interval is %s (waiting %s)", nextInterval, delay)
timer.Reset(delay)
case <-ctx.Done():
return ctx.Err()
Expand All @@ -132,14 +132,14 @@

peers := s.peerTracker.suggestPeers()
start := s.poller.NextInstance
s.Log.Debugf("polling %d peers for instance %d", len(peers), s.poller.NextInstance)
log.Debugf("polling %d peers for instance %d", len(peers), s.poller.NextInstance)
for _, peer := range peers {
oldInstance := s.poller.NextInstance
res, err := s.poller.Poll(ctx, peer)
if err != nil {
return s.poller.NextInstance - start, err
}
s.Log.Debugf("polled %s for instance %d, got %+v", peer, s.poller.NextInstance, res)
log.Debugf("polled %s for instance %d, got %+v", peer, s.poller.NextInstance, res)
// If we manage to advance, all old "hits" are actually misses.
if oldInstance < s.poller.NextInstance {
misses = append(misses, hits...)
Expand Down
2 changes: 0 additions & 2 deletions certexchange/polling/subscriber_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ func TestSubscriber(t *testing.T) {
NetworkName: polling.TestNetworkName,
Host: h,
Store: cs,
Log: polling.TestLog,
}
}

Expand All @@ -63,7 +62,6 @@ func TestSubscriber(t *testing.T) {
client := certexchange.Client{
Host: clientHost,
NetworkName: polling.TestNetworkName,
Log: polling.TestLog,
}

subscriber := polling.Subscriber{
Expand Down
5 changes: 0 additions & 5 deletions certexchange/protocol_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,12 @@ import (

"github.com/ipfs/go-datastore"
ds_sync "github.com/ipfs/go-datastore/sync"
logging "github.com/ipfs/go-log/v2"
mocknetwork "github.com/libp2p/go-libp2p/p2p/net/mock"
"github.com/stretchr/testify/require"
)

const testNetworkName gpbft.NetworkName = "testnet"

var log = logging.Logger("certexchange-test")

func testPowerTable(entries int64) (gpbft.PowerEntries, gpbft.CID) {
powerTable := make(gpbft.PowerEntries, entries)

Expand Down Expand Up @@ -68,13 +65,11 @@ func TestClientServer(t *testing.T) {
NetworkName: testNetworkName,
Host: h1,
Store: cs,
Log: log,
}

client := certexchange.Client{
Host: h2,
NetworkName: testNetworkName,
Log: log,
}

require.NoError(t, server.Start())
Expand Down
18 changes: 10 additions & 8 deletions certexchange/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@
"runtime/debug"
"time"

"github.com/filecoin-project/go-f3"
"github.com/filecoin-project/go-f3/certstore"
"github.com/filecoin-project/go-f3/gpbft"

logging "github.com/ipfs/go-log/v2"
"github.com/libp2p/go-libp2p/core/host"
"github.com/libp2p/go-libp2p/core/network"
)

var log = logging.Logger("f3/certexchange")

const maxResponseLen = 256

// Server is libp2p a certificate exchange server.
Expand All @@ -24,7 +27,6 @@
NetworkName gpbft.NetworkName
Host host.Host
Store *certstore.Store
Log f3.Logger

cancel context.CancelFunc
}
Expand All @@ -40,7 +42,7 @@
defer func() {
if perr := recover(); perr != nil {
_err = fmt.Errorf("panicked in server response: %v", perr)
s.Log.Errorf("%s\n%s", string(debug.Stack()))
log.Errorf("%s\n%s", string(debug.Stack()))

Check warning on line 45 in certexchange/server.go

View check run for this annotation

Codecov / codecov/patch

certexchange/server.go#L45

Added line #L45 was not covered by tests
}
}()

Expand All @@ -56,7 +58,7 @@
// Request has no variable-length fields, so we don't need a limited reader.
var req Request
if err := req.UnmarshalCBOR(br); err != nil {
s.Log.Debugf("failed to read request from stream: %w", err)
log.Debugf("failed to read request from stream: %w", err)

Check warning on line 61 in certexchange/server.go

View check run for this annotation

Codecov / codecov/patch

certexchange/server.go#L61

Added line #L61 was not covered by tests
return err
}

Expand All @@ -72,14 +74,14 @@
if resp.PendingInstance >= req.FirstInstance && req.IncludePowerTable {
pt, err := s.Store.GetPowerTable(ctx, req.FirstInstance)
if err != nil {
s.Log.Errorf("failed to load power table: %w", err)
log.Errorf("failed to load power table: %w", err)

Check warning on line 77 in certexchange/server.go

View check run for this annotation

Codecov / codecov/patch

certexchange/server.go#L77

Added line #L77 was not covered by tests
return err
}
resp.PowerTable = pt
}

if err := resp.MarshalCBOR(bw); err != nil {
s.Log.Debugf("failed to write header to stream: %w", err)
log.Debugf("failed to write header to stream: %w", err)

Check warning on line 84 in certexchange/server.go

View check run for this annotation

Codecov / codecov/patch

certexchange/server.go#L84

Added line #L84 was not covered by tests
return err
}

Expand All @@ -96,12 +98,12 @@
if err == nil || errors.Is(err, certstore.ErrCertNotFound) {
for i := range certs {
if err := certs[i].MarshalCBOR(bw); err != nil {
s.Log.Debugf("failed to write certificate to stream: %w", err)
log.Debugf("failed to write certificate to stream: %w", err)

Check warning on line 101 in certexchange/server.go

View check run for this annotation

Codecov / codecov/patch

certexchange/server.go#L101

Added line #L101 was not covered by tests
return err
}
}
} else {
s.Log.Errorf("failed to load finality certificates: %w", err)
log.Errorf("failed to load finality certificates: %w", err)

Check warning on line 106 in certexchange/server.go

View check run for this annotation

Codecov / codecov/patch

certexchange/server.go#L106

Added line #L106 was not covered by tests
}
}
return bw.Flush()
Expand Down
7 changes: 3 additions & 4 deletions cmd/f3/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
"golang.org/x/xerrors"
)

const DiscoveryTag = "f3-standalone"
var log = logging.Logger("f3/cli")

var log = logging.Logger("f3")
const DiscoveryTag = "f3-standalone"

var runCmd = cli.Command{
Name: "run",
Expand Down Expand Up @@ -115,8 +115,7 @@

ec := ec.NewFakeEC(1, m.BootstrapEpoch, m.ECPeriod, initialPowerTable, true)

module, err := f3.New(ctx, mprovider, ds, h, ps,
signingBackend, ec, log, nil)
module, err := f3.New(ctx, mprovider, ds, h, ps, signingBackend, ec, nil)

Check warning on line 118 in cmd/f3/run.go

View check run for this annotation

Codecov / codecov/patch

cmd/f3/run.go#L118

Added line #L118 was not covered by tests
if err != nil {
return xerrors.Errorf("creating module: %w", err)
}
Expand Down
10 changes: 4 additions & 6 deletions f3.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
host host.Host
ds datastore.Datastore
ec ec.Backend
log Logger
pubsub *pubsub.PubSub

runningCtx context.Context
Expand All @@ -48,7 +47,7 @@
// The context is used for initialization not runtime.
// signingMarshaller can be nil for default SigningMarshaler
func New(_ctx context.Context, manifest manifest.ManifestProvider, ds datastore.Datastore, h host.Host,
ps *pubsub.PubSub, verif gpbft.Verifier, ec ec.Backend, log Logger, signingMarshaller gpbft.SigningMarshaler) (*F3, error) {
ps *pubsub.PubSub, verif gpbft.Verifier, ec ec.Backend, signingMarshaller gpbft.SigningMarshaler) (*F3, error) {
runningCtx, cancel := context.WithCancel(context.Background())
errgrp, runningCtx := errgroup.WithContext(runningCtx)

Expand All @@ -63,7 +62,6 @@
host: h,
ds: ds,
ec: ec,
log: log,
pubsub: ps,
runningCtx: runningCtx,
cancelCtx: cancel,
Expand Down Expand Up @@ -96,13 +94,13 @@
m.mu.Unlock()

if runner == nil {
m.log.Error("attempted to broadcast message while F3 wasn't running")
log.Error("attempted to broadcast message while F3 wasn't running")

Check warning on line 97 in f3.go

View check run for this annotation

Codecov / codecov/patch

f3.go#L97

Added line #L97 was not covered by tests
return
}

err := runner.BroadcastMessage(msg)
if err != nil {
m.log.Errorf("failed to broadcast message: %+v", err)
log.Errorf("failed to broadcast message: %+v", err)

Check warning on line 103 in f3.go

View check run for this annotation

Codecov / codecov/patch

f3.go#L103

Added line #L103 was not covered by tests
}
}

Expand Down Expand Up @@ -258,7 +256,7 @@
m.cs = cs
m.manifest = manifest
m.runner, err = newRunner(
ctx, m.cs, runnerEc, m.log, m.pubsub,
ctx, m.cs, runnerEc, m.pubsub,
m.signingMarshaller, m.verifier,
m.busBroadcast.Publish, m.manifest,
)
Expand Down
Loading
Loading