Fix core indexer logging#481
Conversation
|
|
||
| func (ci *CoreIndexer) Start(ctx context.Context) error { | ||
| sdk := sdk.NewAudiusdSDK(ci.Config.AudiusdURL) | ||
| go logging.SyncOnTicks(ctx, ci.logger, time.Second*10) |
There was a problem hiding this comment.
@rickyrombo didn't i say something about the caller forgetting to do this 😤
There was a problem hiding this comment.
To balance the argument: I like the fiber model where it flushes at the end of the request using a middleware.
For indexing I think I may update this to flush after each block so we don't need to run a ticker at all.
But I do agree that this bit me and kept biting me and I didn't notice until I went to find the logs and found only bite marks.
There was a problem hiding this comment.
i feel like syncing on ticks is an antipattern that we're abusing in both indexers... but maybe it should be opt-out instead of opt-in tho?
There was a problem hiding this comment.
What makes it an anti-pattern? I think there is probably value in flushing on a ticker if the code that's generating the logs may take a long time to reach whatever point it's flushing at. And it feels difficult to pick the right spot in forever loop that's processing events without just sprinkling calls to sync() in a bunch of places and hoping you don't miss somewhere important.
It also feels like it mimics the sidecar behavior you would expect elsewhere. I was surprised to learn that I had to manually flush these logs...
There was a problem hiding this comment.
Maybe not an anti pattern but just kinda like a lazy workaround... Like, you wuldn't flushes SQL statements on a ticker. But it's logs so w/e
3 participants
Based on #258
Adds tick for syncing, sync on close, and fixes a couple of spots where we are direct exiting instead of returning an error and letting the outer loop panic().
My fault for not checking the AI's work hard enough the first time around :-D