report backchannel events #57

merlinran · 2021-10-04T14:46:15Z

No description provided.

merlinran

solicit some thoughts

merlinran · 2021-10-04T14:52:59Z

proto/v1/message.proto

+
+
+message BidBotEvent {
+  google.protobuf.Timestamp ts = 1;


Adding this considering: 1) it may take longer than anticipated time for the event to arrive auctioneer; 2) the local time where bidbot runs may not be synchronized. Both are very unlikely, so not sure if it's worth adding this field for every message.

Imagine a timeline of events for a bid showing up on the UI, for consistency we may prefer to show the timestamp the events being received by auctioneer. your thoughts @asutula ?

Imagine a timeline of events for a bid showing up on the UI, for consistency we may prefer to show the timestamp the events being received by auctioneer.

💯

I'd say it's better to over collect here so we have the options of displaying either or both of the timestamps (bidbot and auctioneer).

merlinran · 2021-10-04T14:54:10Z

proto/v1/message.proto

+    string semantic_version = 1;
+    string storage_provider_id = 2;
+    uint64 deal_start_window = 3;
+    bool cid_gravity_configured = 4;


a few flags we care about most. anything missing?

Moving storage_provider_id to the top level BidBotEvent may make life easier in any way for auctioneer?
At least is self-contained and we don't need PeerID mappings to understand from where is an event (I mean for other oneof that don't contain the provider id).

Yeah good point. I'll see if auctioneer need this field for storing data or logging when implementing this.

Yea good point @jsign. I was also wondering how auctioneer would know who is sending these events? Is there any auth mechanism? If we already are using libp2p and we know who the connected bitbots are there, would it be better to leverage that channel instead of grpc?

Oh, or are these protos going to be sent over libp2p? Maybe I just answered my own question.

Yes they are sent over libp2p.

merlinran · 2021-10-04T14:54:53Z

proto/v1/message.proto

+  oneof type {
+    StartUp start_up = 2;
+    StartFetching start_fetching = 3;
+    ErrorFetching error_fetching = 4;


There's no DoneFetching because it's immediately followed by StartImporting

merlinran · 2021-10-04T14:55:24Z

proto/v1/message.proto

+    string bid_id = 1;
+    uint32 attempts = 2;
+  };
+  message EndImporting {


EndImporting can be either success or error.

Signed-off-by: Merlin Ran <merlinran@gmail.com>

merlinran

This PR does the following:

Added protobuf definitions about the events. already reviewed.
Added a global topic for the events.
Refactored the code to extract the libp2p pubsub code to a separate file, for easier adding features.
Finally, code to fire up the events.

While testing with make up, I noticed a weird issue - the first auction ran fine, but the subsequent ones kept failing, because auctioneer timed out getting the response to the winning bid message, even though bidbot apparently sent it back. Eventually I found out it is not new in this PR, but rather introduced somewhere between v0.1.0 and the current main tip. Will debug as the first thing as it could affect miners if it's a real issue.

merlinran · 2021-10-07T00:38:53Z

go.mod

@@ -40,7 +39,6 @@ require (
 	github.com/textileio/crypto v0.0.0-20210929130053-08edebc3361a
 	github.com/textileio/go-datastore-extensions v1.0.1
 	github.com/textileio/go-ds-badger3 v0.0.0-20210324034212-7b7fb3be3d1c
-	github.com/textileio/go-ds-mongo v0.1.4


removed dependency to mongo, which by chance has https://github.com/textileio/bidbot/security/dependabot/go.sum/go.mongodb.org%2Fmongo-driver/open

merlinran · 2021-10-07T00:40:25Z

lib/auction/topics.go

 const Topic string = "/textile/auction/0.0.1"

+// BidbotEventsTopic is used by bidbots to notify auctioneers various events, mainly around the lifecycle of bids.
+var BidbotEventsTopic string = path.Join(Topic, "bidbot_events")


Initially I created one topic per bidbot, but it's tricky because auctioneer need to subscribe to them before they fire up the startup message. Using a single topic is much easier.

merlinran · 2021-10-07T00:43:12Z

proto/v1/message.proto

+    bool cid_gravity_configured = 4;
+    bool cid_gravity_strict = 5;
+  };
+  message Unhealthy {


Added this one. Can be extended to other problem reporting in the future. Don't like the name though

merlinran · 2021-10-07T00:48:08Z

service/libp2p_pubsub.go

+)
+
+// CommChannel represents the communication channel with auctioneer.
+type CommChannel interface {


Extracted out the pubsub code along the way. I've long thought about doing this, to avoid cluttering business logic with communication details. I didn't touch the tests but later on if we add more complex logic, we can test them without firing up libp2p pubsub instances. Worth a better name though.

merlinran · 2021-10-07T00:51:30Z

service/progress_reporter.go

+	"google.golang.org/protobuf/types/known/timestamppb"
+)
+
+type progressReporter struct {


pretty boring. just compose and fire up the protobuf messages.

merlinran · 2021-10-07T00:52:42Z

service/service.go

@@ -163,7 +163,6 @@ type Service struct {

 	ctx       context.Context
 	finalizer *finalizer.Finalizer
-	lk        sync.Mutex


This lock was removed when extracting the libp2p pubsub code. It was for locking Subscribe, but it's only called once, so became useless.

merlinran · 2021-10-07T00:55:30Z

service/service.go

-		s.peer.Bootstrap()
+	err := s.commChannel.Subscribe(bootstrap, s)
+	if err == nil {
+		time.AfterFunc(30*time.Second, s.reportStartup)


It wasted me hours trying to find a way to see if auctioneer is connected. no luck, falls back to this hack. If bidbot doesn't hold for 30s, it probably shouldn't be considered started up.

Probably I might not be totally understanding, but can't host.Network().Connectedness(auctioneersPeerID) help answer that question?

I didn't know this one actually, thanks! Follow this path I see I can also register a Notifiee via host.Network().Notify() to avoid polling, but that involves more code. Maybe not bad to lean on the simpler side?

Glad it was useful! Yeah, I'd lean on the simpler side. Not a big deal.

merlinran · 2021-10-07T00:56:59Z

service/service.go

 	pcid, err := cid.Decode(prop.ProposalCid)
 	if err != nil {
-		log.Errorf("decoding proposal cid: %v", err)


error is printed by the caller of ProposalsHandler now

merlinran · 2021-10-07T01:00:04Z

service/service.go

 				}
-				log.Infof("bidbot up %v, %d connected peers",
-					time.Since(startAt), len(s.peer.ListPeers()))


This was not very useful - just puzzled miners. Removed this and renamed the method.

merlinran · 2021-10-07T01:01:35Z

service/store/store.go

@@ -554,6 +571,7 @@ func (s *Store) fetchWorker(num int) {
 		if b.DataURIFetchAttempts >= s.dealDataFetchAttempts {
 			status = BidStatusErrored
 			s.bytesLimiter.Withdraw(string(b.AuctionID))
+			s.dealProgressReporter.Errored(b.ID, b.ErrorCause)


These are where the events originated.

jsign

Very nice! Looking good.

@merlinran, I'm wondering if worth tagging a version before merging this PR. At least to have a version for the closed auctions that we know some miners are running now and looks safe; and leave these changes for the next version.

jsign · 2021-10-07T12:00:33Z

service/service.go

-		s.peer.Bootstrap()
+	err := s.commChannel.Subscribe(bootstrap, s)
+	if err == nil {
+		time.AfterFunc(30*time.Second, s.reportStartup)


Probably I might not be totally understanding, but can't host.Network().Connectedness(auctioneersPeerID) help answer that question?

jsign · 2021-10-07T12:02:42Z

service/store/store.go

@@ -554,6 +571,7 @@ func (s *Store) fetchWorker(num int) {
 		if b.DataURIFetchAttempts >= s.dealDataFetchAttempts {
 			status = BidStatusErrored
 			s.bytesLimiter.Withdraw(string(b.AuctionID))
+			s.dealProgressReporter.Errored(b.ID, b.ErrorCause)


merlinran · 2021-10-07T14:01:45Z

Good point on cutting a release. Will do after figuring out the weirdness of make up and before merging this.

merlinran · 2021-10-07T14:52:05Z

Okay cut off v0.1.1 for the closed auction @jsign , and verified the make up issue is something with my auction system changes (not found out yet), irrelevant to bidbot (glad that the miners are unaffected!). Merging!

jsign · 2021-10-07T15:10:26Z

Okay cut off v0.1.1 for the closed auction @jsign , and verified the make up issue is something with my auction system changes (not found out yet), irrelevant to bidbot (glad that the miners are unaffected!). Merging!

Excellent, I'll mention that tag in the notion doc where we refer to the feature. Thanks!

merlinran commented Oct 4, 2021

View reviewed changes

merlinran added 3 commits October 5, 2021 14:47

add protobuf definitions for the backchannel events

1a8ca32

Signed-off-by: Merlin Ran <merlinran@gmail.com>

extract the communication channel to a separate package

e616af8

Signed-off-by: Merlin Ran <merlinran@gmail.com>

report various events back to auctioneer

5769ff6

Signed-off-by: Merlin Ran <merlinran@gmail.com>

merlinran force-pushed the merlin/backchannel-event branch from 9e9cfaf to 5769ff6 Compare October 5, 2021 19:40

remove the unused mongo store helper

c936fbe

Signed-off-by: Merlin Ran <merlinran@gmail.com>

merlinran force-pushed the merlin/backchannel-event branch 4 times, most recently from 25ccef2 to 1e55743 Compare October 6, 2021 13:49

rename for better [shipit]

39386ef

Signed-off-by: Merlin Ran <merlinran@gmail.com>

merlinran force-pushed the merlin/backchannel-event branch 4 times, most recently from 8df3dba to 213f80b Compare October 7, 2021 00:02

merlinran changed the title ~~add protobuf definitions for the backchannel events~~ report backchannel events Oct 7, 2021

merlinran force-pushed the merlin/backchannel-event branch 2 times, most recently from 9b40580 to 2c1d907 Compare October 7, 2021 00:41

use a single topic for bidbot events reporting

411dd01

Signed-off-by: Merlin Ran <merlinran@gmail.com>

merlinran force-pushed the merlin/backchannel-event branch from 2c1d907 to 411dd01 Compare October 7, 2021 00:58

merlinran commented Oct 7, 2021

View reviewed changes

merlinran marked this pull request as ready for review October 7, 2021 01:13

jsign approved these changes Oct 7, 2021

View reviewed changes

merlinran merged commit 32d70a4 into main Oct 7, 2021

merlinran deleted the merlin/backchannel-event branch October 7, 2021 14:53

merlinran mentioned this pull request Oct 7, 2021

store the bidbot backchannel events textileio/broker-core#327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report backchannel events #57

report backchannel events #57

merlinran commented Oct 4, 2021

merlinran left a comment

merlinran Oct 4, 2021

jsign Oct 4, 2021

asutula Oct 4, 2021

merlinran Oct 4, 2021

jsign Oct 4, 2021 •

edited

Loading

merlinran Oct 4, 2021

asutula Oct 4, 2021

asutula Oct 4, 2021

merlinran Oct 4, 2021

merlinran Oct 4, 2021

merlinran Oct 4, 2021

merlinran left a comment

merlinran Oct 7, 2021

merlinran Oct 7, 2021

merlinran Oct 7, 2021

merlinran Oct 7, 2021

merlinran Oct 7, 2021

merlinran Oct 7, 2021

merlinran Oct 7, 2021

jsign Oct 7, 2021

merlinran Oct 7, 2021

jsign Oct 7, 2021

merlinran Oct 7, 2021

merlinran Oct 7, 2021

merlinran Oct 7, 2021

jsign Oct 7, 2021

jsign left a comment

jsign Oct 7, 2021

jsign Oct 7, 2021

merlinran commented Oct 7, 2021

merlinran commented Oct 7, 2021

jsign commented Oct 7, 2021

report backchannel events #57

report backchannel events #57

Conversation

merlinran commented Oct 4, 2021

merlinran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsign Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlinran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlinran commented Oct 7, 2021

merlinran commented Oct 7, 2021

jsign commented Oct 7, 2021

jsign Oct 4, 2021 •

edited

Loading