PLT-7497: restructure Marconi with a focus on logging - #212
Conversation
Signed-off-by: Ana Pantilie <ana.pantilie@iohk.io>
neilmayhew
left a comment
neilmayhew
left a comment
There was a problem hiding this comment.
My comments and suggestions here are fairly random and untested, so feel free to ignore them if they don't seem like they will work
| -- TODO: this should also throw an exception | ||
| logMError = undefined |
There was a problem hiding this comment.
I feel like log functions shouldn't be throwing exceptions, despite what we said earlier today about having a unified framework for both.
Perhaps it would be OK to have a function that does both, but have that reflected in the name. Something like logThrowError.
In another system I worked on, we also included a "context" parameter which was just a string that indicated where the error was thrown from. This helped a lot when looking at logs.
There was a problem hiding this comment.
I agree, I think we need to have the concrete requirements regarding errors and exceptions in order to make any changes here.
| -- TODO: this function is problematic because it is used both inside the | ||
| -- ChainIndexT context and outside to initialize the config | ||
| -- see Marconi.ChainIndex.Run.run |
There was a problem hiding this comment.
Could you make the monad be a type parameter m and give it constraints (MonadIO m, MonadUnliftIO m)?
Or have two versions of the function, one of which wraps the other and lives in ChainIndexT?
There was a problem hiding this comment.
I'm not sure about using MonadUnliftIO. If I understand correctly, it looks like we'd be restricting our ChainIndexT monad stack to just having reader capabilites, and I don't have enough knowledge about the domain if we need to worry about this or not.
There was a problem hiding this comment.
Hmm, still, for the second solution I believe you end up explicitly implementing the ChainIndexT -> IO transformation. Unfortunately this isn't the only place in the code this transformation is needed I think, not 100% sure yet but wanted to add to my previous comment.
Signed-off-by: Ana Pantilie <ana.pantilie@iohk.io>
Signed-off-by: Ana Pantilie <ana.pantilie@iohk.io>
neilmayhew
left a comment
neilmayhew
left a comment
There was a problem hiding this comment.
This is coming together nicely
| liftIO | ||
| . Utils.toException | ||
| $ Utils.queryCurrentNodeBlockNo @Void networkId socketPath | ||
| let indexers = mapMaybe sequenceA indexerList | ||
| coordinator <- liftIO $ initializeCoordinatorFromIndexers securityParam indexingDepth indexers | ||
| let indexerDepth = | ||
| let SecurityParam s = securityParam | ||
| in case indexingDepth of | ||
| MinIndexingDepth d -> SecurityParam $ s - d + 1 | ||
| MaxIndexingDepth -> SecurityParam 1 | ||
| resumablePoints <- | ||
| liftIO $ getStartingPointsFromIndexers indexerDepth currentNodeBlockNo indexers coordinator | ||
| let oldestCommonChainPoint = minimum resumablePoints | ||
| resumePoint = case cliChainPoint of | ||
| C.ChainPointAtGenesis -> oldestCommonChainPoint -- User didn't specify a chain point, use oldest common chain point, | ||
| cliCp -> cliCp -- otherwise use what was provided on CLI. |
There was a problem hiding this comment.
I think you could avoid some of the uses of liftIO by moving these lines outside of withNodeConnectRetryMarconi. I don't think any of them have to run in ChainIndexerT.
However, maybe you plan to migrate more of these functions to ChainIndexerT eventually, so they can get their parameters from the reader env. In which case, they would need to stay where they are.
| mkIndexerStream coordinator | ||
| . chainSyncEventStreamLogging | ||
| . updateProcessedBlocksMetric | ||
| runChainSyncStream = liftIO $ withChainSyncBlockEventStream socketPath networkId [resumePoint] stream |
There was a problem hiding this comment.
Now that withChainSyncBlockEventStream runs in MonadIO I don't think you need a liftIO here.
There was a problem hiding this comment.
Overview of one of the problems encountered in my investigation
chainSyncEventStreamLogging needs to have access to the logging context.
The stream is problematic here because it needs to both be in the ChainIndexT context (as noted above) and in IO because it is run by withChainSyncBlockEventStream. Inside, it uses withAsync on the stream consumer which means that we would need a way to "unlift" the ChainIndexT back to IO.
This brings forward the question of whether we can make ChainIndexT an instance of MonadUnliftIO. This would restrict the monad stack to allow only the current reader capability, and also it does not play well with the ExceptT we have decided to keep (as per @willjgould's investigation).
I believe that there are two possible ways to go on from here:
- we make
ChainIndexTan instance ofMonadUnliftIOand see how it impacts the usage ofExceptT, keeping in mind the restrictions we will impose - we refactor the code in such a way that rids it of how deeply coupled it is with IO now, as Will also suggested; I don't know if this is possible, someone with more knowledge of the business logic should have a better idea than me
2 participants
Pre-submit checklist: