Big refactor to clean up the WatchSlots logic #34
Conversation
There was a problem hiding this comment.
I would advise just reading this file plainly as is, as the whole thing was rewritten and so the diff is not very helpful IMO
johnstonematt
force-pushed
the
watch-slots
branch
from
7bf9c8d to
db8373b
Compare
cmd/solana_exporter/utils.go
Outdated
| func assertf(condition bool, format string, args ...any) { | ||
| if !condition { | ||
| panic(fmt.Errorf(format, args...)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of a bare panic with the string, instead use klog.Fatalf(format, args...).
fwiw, generally speaking, for non-safety critical things, I consider panics in code to be code smell. If possible, just lazily loop and try again when feasible instead of hard crashing.
| func (c *SlotWatcher) checkValidSlotRange(from, to int64) error { | ||
| if from < c.firstSlot || to > c.lastSlot { | ||
| return fmt.Errorf( | ||
| "start-end slots (%v -> %v) is not contained within current epoch %v range (%v -> %v)", |
There was a problem hiding this comment.
| "start-end slots (%v -> %v) is not contained within current epoch %v range (%v -> %v)", | |
| "start-end slots (%v -> %v) are not contained within current epoch %v range (%v -> %v)", |
cmd/solana_exporter/slots.go
Outdated
|
|
||
| // make sure the bounds are contained within the epoch we are currently watching: | ||
| if err := c.checkValidSlotRange(startSlot, endSlot); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Is a panic really necessary in this case? Can we not just gracefully skip this or reset things some how? Also, perhaps instead of a panic (which dumps all goroutines + stack traces to stdout, we just do something more like klog.Fatal(err.String()) or maybe klog.Fatalf("slot range is out of bounds of the current epoch: %v", err)
There was a problem hiding this comment.
The reason I panic here is because if it happens then it's a logic issue in the code, not just some random event.
cmd/solana_exporter/utils.go
Outdated
| // For pretty-printed JSON, use json.MarshalIndent | ||
| prettyJSON, err := json.MarshalIndent(obj, "", " ") | ||
| if err != nil { | ||
| fmt.Println("Error marshalling to pretty JSON:", err, ". obj: ", obj) |
There was a problem hiding this comment.
Should this be using something like this? klog.Info("error marshalling to pretty JSON: %s. obj: %v", err, obj)
Bare fmt.Print.. in code that is long running is generally a bug.
| // CommitmentFinalized level offers the highest level of certainty for a transaction on the Solana blockchain. | ||
| // A transaction is considered “Finalized” when it is included in a block that has been confirmed by a | ||
| // supermajority of the stake, and at least 31 additional confirmed blocks have been built on top of it. | ||
| CommitmentFinalized Commitment = "finalized" |
There was a problem hiding this comment.
For the usage of the exporter, do we actually need to use finalized blocks? This is ~15 seconds behind the latest roughly. For monitoring, max is probably good enough.
There was a problem hiding this comment.
The "max" commitment is deprecated, current levels are only "finalized", "confirmed" and "processed", and "max" maps to "finalized", as per this commit
pkg/rpc/client.go
Outdated
| if lastSlot != nil { | ||
| // make sure first and last slot are in order: | ||
| if *firstSlot > *lastSlot { | ||
| panic(fmt.Errorf("last slot %v is greater than first slot %v", *lastSlot, *firstSlot)) |
There was a problem hiding this comment.
Can we just return this as an error instead of panicking? Less panicking is good.
Main Refactor
I replaced the monolithic
WatchSlots()with theSolanaCollector.WatchSlots(), which I personally believe is much better for reasons such as:In general, the logic in the previous one was just insanely hard to follow, and I am 99% certain there were double counting errors on epoch boundaries.
One of the main things I focused on in
SolanaCollector.WatchSlots()was to clearly mark the logic that is executed on epoch boundary, to make it easy to implement things like inflationary and Jito reward tracking, which happens at epoch bounds.Other changes:
rpc.Clientto make the functions more like a direct overlay of the RPC API, without any additional logic.Commitmentlevels, as the ones used have been deprecated for a while.Still to come:
With the logic split up more granularly now, more tests can be done.