Add Enclave block and Batch health checks #1727
Conversation
WalkthroughThe overall change enhances the system's health check mechanisms by introducing thread-safe timestamp management and incorporating health check methods across various components. The Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (6)
- go/common/async/timestamp.go (1 hunks)
- go/enclave/components/batch_registry.go (5 hunks)
- go/enclave/components/block_processor.go (4 hunks)
- go/enclave/components/interfaces.go (2 hunks)
- go/enclave/enclave.go (1 hunks)
- go/enclave/storage/storage.go (1 hunks)
Additional comments: 17
go/common/async/timestamp.go (4)
8-12: The
Timestampstruct is well-defined and thread-safe due to the use ofsync.RWMutex.14-18: The constructor
NewAsyncTimestampcorrectly initializes theTimestampstruct with the providedlastTimestampand a newsync.RWMutex.21-26: The
Markmethod correctly locks the mutex for writing and updates thelastTimestampwith the current time. The use ofdeferensures the mutex is unlocked even if a panic occurs.28-34: The
LastTimestampmethod correctly locks the mutex for reading and returns a copy oflastTimestamp. The use ofdeferis good practice to ensure the mutex is unlocked.go/enclave/components/interfaces.go (2)
40-40: The
HealthCheckmethod has been added to theL1BlockProcessorinterface, which is expected to return a boolean status and an error. This aligns with the PR objectives.107-107: Similarly, the
HealthCheckmethod has been added to theBatchRegistryinterface with the same return signature, which is consistent and expected.go/enclave/components/block_processor.go (5)
6-6: The import of the
timepackage is necessary for the new timestamp-related functionality.31-33: New fields
healthTimeoutandlastIngestedBlockhave been added to thel1BlockProcessorstruct to support health check functionality.54-55: The
NewBlockProcessorfunction initializes the new fieldshealthTimeoutandlastIngestedBlockwith sensible defaults. ThelastIngestedBlockis initialized with a timestamp one minute in the past, which may be a safe assumption to avoid immediate health check failure on startup.85-85: The
Processfunction now callsMarkonlastIngestedBlockto update the timestamp when a block is processed, which is crucial for the health check mechanism.89-95: The
HealthCheckmethod correctly compares the current time with thelastIngestedBlockTimeplushealthTimeoutto determine if the last block ingestion is within the expected time frame. The error message provides useful information about the delay.go/enclave/components/batch_registry.go (5)
8-8: The import of the
timepackage is necessary for the new timestamp-related functionality.31-32: New fields
healthTimeoutandlastExecutedBatchhave been added to thebatchRegistrystruct to support health check functionality.52-53: The
NewBatchRegistryfunction initializes the new fieldshealthTimeoutandlastExecutedBatchwith sensible defaults. ThelastExecutedBatchis initialized with a timestamp one minute in the past, which may be a safe assumption to avoid immediate health check failure on startup.85-85: The
OnBatchExecutedmethod now callsMarkonlastExecutedBatchto update the timestamp when a batch is executed, which is crucial for the health check mechanism.205-211: The
HealthCheckmethod correctly compares the current time with thelastExecutedBatchTimeplushealthTimeoutto determine if the last batch execution is within the expected time frame. The error message provides useful information about the delay.go/enclave/storage/storage.go (1)
- 266-276: The addition of a nil check for
headBatchin theHealthCheckmethod is a good practice to prevent potential nil pointer dereferences. This change ensures that the method returns an error if theheadBatchis nil, which is a clear indication of an unhealthy state.if headBatch == nil { + return false, fmt.Errorf("head batch is nil") }
|
|
||
| // todo (#1148) - enclave healthcheck operations | ||
| enclaveHealthy := true | ||
| return storageHealthy && enclaveHealthy, nil | ||
| l1blockHealthy, err := e.l1BlockProcessor.HealthCheck() | ||
| if err != nil { | ||
| // simplest iteration, log the error and just return that it's not healthy | ||
| e.logger.Info("HealthCheck failed for the l1 block processor", log.ErrKey, err) | ||
| return false, nil | ||
| } | ||
|
|
||
| l2batchHealthy, err := e.registry.HealthCheck() | ||
| if err != nil { | ||
| // simplest iteration, log the error and just return that it's not healthy | ||
| e.logger.Info("HealthCheck failed for the l2 batch registry", log.ErrKey, err) | ||
| return false, nil | ||
| } | ||
|
|
||
| return storageHealthy && l1blockHealthy && l2batchHealthy, nil |
There was a problem hiding this comment.
The implementation of the HealthCheck method has been updated to include checks for l1BlockProcessor and registry. Ensure that the error handling and logging are consistent and that the method's behavior aligns with the intended health check logic.
func (e *enclaveImpl) HealthCheck() (bool, common.SystemError) {
if e.stopControl.IsStopping() {
return false, responses.ToInternalError(fmt.Errorf("requested HealthCheck with the enclave stopping"))
}
// check the storage health
storageHealthy, err := e.storage.HealthCheck()
if err != nil {
e.logger.Info("HealthCheck failed for the enclave storage", log.ErrKey, err)
return false, nil
}
l1blockHealthy, err := e.l1BlockProcessor.HealthCheck()
if err != nil {
e.logger.Info("HealthCheck failed for the l1 block processor", log.ErrKey, err)
return false, nil
}
l2batchHealthy, err := e.registry.HealthCheck()
if err != nil {
e.logger.Info("HealthCheck failed for the l2 batch registry", log.ErrKey, err)
return false, nil
}
return storageHealthy && l1blockHealthy && l2batchHealthy, nil
}Consider enhancing the error handling to provide more detailed information about which component failed the health check. Additionally, the log level for health check failures should be more severe than Info, such as Warn or Error, to reflect the critical nature of health check failures.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // todo (#1148) - enclave healthcheck operations | |
| enclaveHealthy := true | |
| return storageHealthy && enclaveHealthy, nil | |
| l1blockHealthy, err := e.l1BlockProcessor.HealthCheck() | |
| if err != nil { | |
| // simplest iteration, log the error and just return that it's not healthy | |
| e.logger.Info("HealthCheck failed for the l1 block processor", log.ErrKey, err) | |
| return false, nil | |
| } | |
| l2batchHealthy, err := e.registry.HealthCheck() | |
| if err != nil { | |
| // simplest iteration, log the error and just return that it's not healthy | |
| e.logger.Info("HealthCheck failed for the l2 batch registry", log.ErrKey, err) | |
| return false, nil | |
| } | |
| return storageHealthy && l1blockHealthy && l2batchHealthy, nil | |
| // todo (#1148) - enclave healthcheck operations | |
| if e.stopControl.IsStopping() { | |
| return false, responses.ToInternalError(fmt.Errorf("requested HealthCheck with the enclave stopping")) | |
| } | |
| // check the storage health | |
| storageHealthy, err := e.storage.HealthCheck() | |
| if err != nil { | |
| e.logger.Warn("HealthCheck failed for the enclave storage", log.ErrKey, err) | |
| return false, nil | |
| } | |
| l1blockHealthy, err := e.l1BlockProcessor.HealthCheck() | |
| if err != nil { | |
| e.logger.Warn("HealthCheck failed for the l1 block processor", log.ErrKey, err) | |
| return false, nil | |
| } | |
| l2batchHealthy, err := e.registry.HealthCheck() | |
| if err != nil { | |
| e.logger.Warn("HealthCheck failed for the l2 batch registry", log.ErrKey, err) | |
| return false, nil | |
| } | |
| return storageHealthy && l1blockHealthy && l2batchHealthy, nil |
| e.logger.Info("HealthCheck failed for the enclave storage", log.ErrKey, err) | ||
| return false, nil | ||
| } | ||
|
|
||
| // todo (#1148) - enclave healthcheck operations | ||
| enclaveHealthy := true | ||
| return storageHealthy && enclaveHealthy, nil | ||
| l1blockHealthy, err := e.l1BlockProcessor.HealthCheck() | ||
| if err != nil { | ||
| // simplest iteration, log the error and just return that it's not healthy | ||
| e.logger.Info("HealthCheck failed for the l1 block processor", log.ErrKey, err) | ||
| return false, nil | ||
| } | ||
|
|
||
| l2batchHealthy, err := e.registry.HealthCheck() | ||
| if err != nil { | ||
| // simplest iteration, log the error and just return that it's not healthy | ||
| e.logger.Info("HealthCheck failed for the l2 batch registry", log.ErrKey, err) | ||
| return false, nil | ||
| } | ||
|
|
||
| return storageHealthy && l1blockHealthy && l2batchHealthy, nil |
There was a problem hiding this comment.
The HealthCheck method has been updated to include additional health checks for l1BlockProcessor and registry. Ensure that the error handling and logging are consistent and that the method's behavior aligns with the intended health check logic.
func (e *enclaveImpl) HealthCheck() (bool, common.SystemError) {
if e.stopControl.IsStopping() {
return false, responses.ToInternalError(fmt.Errorf("requested HealthCheck with the enclave stopping"))
}
// check the storage health
storageHealthy, err := e.storage.HealthCheck()
if err != nil {
e.logger.Info("HealthCheck failed for the enclave storage", log.ErrKey, err)
return false, nil
}
l1blockHealthy, err := e.l1BlockProcessor.HealthCheck()
if err != nil {
e.logger.Info("HealthCheck failed for the l1 block processor", log.ErrKey, err)
return false, nil
}
l2batchHealthy, err := e.registry.HealthCheck()
if err != nil {
e.logger.Info("HealthCheck failed for the l2 batch registry", log.ErrKey, err)
return false, nil
}
return storageHealthy && l1blockHealthy && l2batchHealthy, nil
}Consider enhancing the error handling to provide more detailed information about which component failed the health check. Additionally, the log level for health check failures should be more severe than Info, such as Warn or Error, to reflect the critical nature of health check failures.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| e.logger.Info("HealthCheck failed for the enclave storage", log.ErrKey, err) | |
| return false, nil | |
| } | |
| // todo (#1148) - enclave healthcheck operations | |
| enclaveHealthy := true | |
| return storageHealthy && enclaveHealthy, nil | |
| l1blockHealthy, err := e.l1BlockProcessor.HealthCheck() | |
| if err != nil { | |
| // simplest iteration, log the error and just return that it's not healthy | |
| e.logger.Info("HealthCheck failed for the l1 block processor", log.ErrKey, err) | |
| return false, nil | |
| } | |
| l2batchHealthy, err := e.registry.HealthCheck() | |
| if err != nil { | |
| // simplest iteration, log the error and just return that it's not healthy | |
| e.logger.Info("HealthCheck failed for the l2 batch registry", log.ErrKey, err) | |
| return false, nil | |
| } | |
| return storageHealthy && l1blockHealthy && l2batchHealthy, nil | |
| e.logger.Warn("HealthCheck failed for the enclave storage", log.ErrKey, err) | |
| return false, responses.ToInternalError(err) | |
| } | |
| // todo (#1148) - enclave healthcheck operations | |
| l1blockHealthy, err := e.l1BlockProcessor.HealthCheck() | |
| if err != nil { | |
| e.logger.Warn("HealthCheck failed for the l1 block processor", log.ErrKey, err) | |
| return false, responses.ToInternalError(err) | |
| } | |
| l2batchHealthy, err := e.registry.HealthCheck() | |
| if err != nil { | |
| e.logger.Warn("HealthCheck failed for the l2 batch registry", log.ErrKey, err) | |
| return false, responses.ToInternalError(err) | |
| } | |
| return storageHealthy && l1blockHealthy && l2batchHealthy, nil |
The HealthCheck method has been updated to include additional health checks for l1BlockProcessor and registry. Ensure that the error handling and logging are consistent and that the method's behavior aligns with the intended health check logic.
func (e *enclaveImpl) HealthCheck() (bool, common.SystemError) {
if e.stopControl.IsStopping() {
return false, responses.ToInternalError(fmt.Errorf("requested HealthCheck with the enclave stopping"))
}
// check the storage health
storageHealthy, err := e.storage.HealthCheck()
if err != nil {
e.logger.Info("HealthCheck failed for the enclave storage", log.ErrKey, err)
return false, nil
}
l1blockHealthy, err := e.l1BlockProcessor.HealthCheck()
if err != nil {
e.logger.Info("HealthCheck failed for the l1 block processor", log.ErrKey, err)
return false, nil
}
l2batchHealthy, err := e.registry.HealthCheck()
if err != nil {
e.logger.Info("HealthCheck failed for the l2 batch registry", log.ErrKey, err)
return false, nil
}
return storageHealthy && l1blockHealthy && l2batchHealthy, nil
}Consider enhancing the error handling to provide more detailed information about which component failed the health check. Additionally, the log level for health check failures should be more severe than Info, such as Warn or Error, to reflect the critical nature of health check failures.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| e.logger.Info("HealthCheck failed for the enclave storage", log.ErrKey, err) | |
| return false, nil | |
| } | |
| // todo (#1148) - enclave healthcheck operations | |
| enclaveHealthy := true | |
| return storageHealthy && enclaveHealthy, nil | |
| l1blockHealthy, err := e.l1BlockProcessor.HealthCheck() | |
| if err != nil { | |
| // simplest iteration, log the error and just return that it's not healthy | |
| e.logger.Info("HealthCheck failed for the l1 block processor", log.ErrKey, err) | |
| return false, nil | |
| } | |
| l2batchHealthy, err := e.registry.HealthCheck() | |
| if err != nil { | |
| // simplest iteration, log the error and just return that it's not healthy | |
| e.logger.Info("HealthCheck failed for the l2 batch registry", log.ErrKey, err) | |
| return false, nil | |
| } | |
| return storageHealthy && l1blockHealthy && l2batchHealthy, nil | |
| e.logger.Warn("HealthCheck failed for the enclave storage", log.ErrKey, err) | |
| return false, responses.ToInternalError(err) | |
| } | |
| l1blockHealthy, err := e.l1BlockProcessor.HealthCheck() | |
| if err != nil { | |
| e.logger.Warn("HealthCheck failed for the l1 block processor", log.ErrKey, err) | |
| return false, responses.ToInternalError(err) | |
| } | |
| l2batchHealthy, err := e.registry.HealthCheck() | |
| if err != nil { | |
| e.logger.Warn("HealthCheck failed for the l2 batch registry", log.ErrKey, err) | |
| return false, responses.ToInternalError(err) | |
| } | |
| return storageHealthy && l1blockHealthy && l2batchHealthy, nil |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- go/enclave/components/batch_registry.go (5 hunks)
- go/enclave/components/block_processor.go (4 hunks)
- go/enclave/txpool/txpool_mock_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- go/enclave/components/batch_registry.go
- go/enclave/components/block_processor.go
| func (m *mockBatchRegistry) HealthCheck() (bool, error) { | ||
| // TODO implement me | ||
| panic("implement me") | ||
| } |
There was a problem hiding this comment.
The HealthCheck method in mockBatchRegistry is not implemented and will cause a panic if called. Consider implementing the method or updating the TODO comment with more details about the intended implementation.
func (m *mockBatchRegistry) HealthCheck() (bool, error) {
- // TODO implement me
- panic("implement me")
+ // TODO: Implement the health check logic or return mock values for testing purposes.
+ // return true, nil // Example mock implementation
}Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (m *mockBatchRegistry) HealthCheck() (bool, error) { | |
| // TODO implement me | |
| panic("implement me") | |
| } | |
| func (m *mockBatchRegistry) HealthCheck() (bool, error) { | |
| // TODO: Implement the health check logic or return mock values for testing purposes. | |
| // return true, nil // Example mock implementation | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration/simulation/simulation.go (2 hunks)
Additional comments: 1
integration/simulation/simulation.go (1)
- 7-7: The addition of the
retrypackage is consistent with the changes in thecheckHealthStatusmethod to implement a retry mechanism.
| err := retry.Do(func() error { | ||
| healthy, err := client.Health() | ||
| if !healthy || err != nil { | ||
| return fmt.Errorf("client is not healthy: %w", err) | ||
| } | ||
| return nil | ||
| }, retry.NewTimeoutStrategy(30*time.Second, 100*time.Millisecond)) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
The implementation of the retry mechanism in checkHealthStatus uses a timeout strategy, which is a good practice for network-related operations that might occasionally fail. However, ensure that the panic does not cause the entire application to crash in a production environment. It might be more appropriate to handle the error gracefully and log it for further investigation.
- panic(err)
+ // Log the error and handle it appropriately without causing a system-wide failure.
+ log.Error("Health check failed", "error", err)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| err := retry.Do(func() error { | |
| healthy, err := client.Health() | |
| if !healthy || err != nil { | |
| return fmt.Errorf("client is not healthy: %w", err) | |
| } | |
| return nil | |
| }, retry.NewTimeoutStrategy(30*time.Second, 100*time.Millisecond)) | |
| if err != nil { | |
| panic(err) | |
| err := retry.Do(func() error { | |
| healthy, err := client.Health() | |
| if !healthy || err != nil { | |
| return fmt.Errorf("client is not healthy: %w", err) | |
| } | |
| return nil | |
| }, retry.NewTimeoutStrategy(30*time.Second, 100*time.Millisecond)) | |
| if err != nil { | |
| // Log the error and handle it appropriately without causing a system-wide failure. | |
| log.Error("Health check failed", "error", err) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration/simulation/simulation.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration/simulation/simulation.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/enclave/components/batch_registry.go (5 hunks)
- go/enclave/components/block_processor.go (4 hunks)
Additional comments: 8
go/enclave/components/block_processor.go (4)
6-6: The import of the
timepackage is necessary for the new timestamp-related features.31-33: The addition of
healthTimeoutandlastIngestedBlockfields to thel1BlockProcessorstruct aligns with the PR's objective to enhance health check mechanisms.54-55: Initialization of
healthTimeouttotime.MinuteandlastIngestedBlockto one minute in the past ensures immediate readiness for health checks post-instantiation.85-85: The call to
lastIngestedBlock.Mark()in theProcessfunction correctly updates the timestamp when a new block is ingested, which is crucial for the health check mechanism.go/enclave/components/batch_registry.go (4)
8-8: The import of the
timepackage is necessary for the new timestamp-related features.31-32: The addition of
healthTimeoutandlastExecutedBatchfields to thebatchRegistrystruct aligns with the PR's objective to enhance health check mechanisms.52-53: Initialization of
healthTimeouttotime.MinuteandlastExecutedBatchto one minute in the past ensures immediate readiness for health checks post-instantiation.85-85: The call to
lastExecutedBatch.Mark()in theOnBatchExecutedmethod correctly updates the timestamp when a batch is executed, which is crucial for the health check mechanism.
| // HealthCheck checks if the last ingested block was more than healthTimeout ago | ||
| func (bp *l1BlockProcessor) HealthCheck() (bool, error) { | ||
| lastIngestedBlockTime := bp.lastIngestedBlock.LastTimestamp() | ||
| if time.Now().After(lastIngestedBlockTime.Add(bp.healthTimeout)) { | ||
| return false, fmt.Errorf("last ingested block was %s ago", time.Since(lastIngestedBlockTime)) | ||
| } | ||
|
|
||
| return true, nil | ||
| } |
There was a problem hiding this comment.
The HealthCheck method correctly assesses the health based on the time elapsed since the last ingested block. However, consider adding a comment explaining what constitutes a healthy or unhealthy state, as suggested by a previous reviewer.
+ // HealthCheck determines if the processor is healthy based on the time since the last block ingestion.Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // HealthCheck checks if the last ingested block was more than healthTimeout ago | |
| func (bp *l1BlockProcessor) HealthCheck() (bool, error) { | |
| lastIngestedBlockTime := bp.lastIngestedBlock.LastTimestamp() | |
| if time.Now().After(lastIngestedBlockTime.Add(bp.healthTimeout)) { | |
| return false, fmt.Errorf("last ingested block was %s ago", time.Since(lastIngestedBlockTime)) | |
| } | |
| return true, nil | |
| } | |
| // HealthCheck determines if the processor is healthy based on the time since the last block ingestion. | |
| // HealthCheck checks if the last ingested block was more than healthTimeout ago | |
| func (bp *l1BlockProcessor) HealthCheck() (bool, error) { | |
| lastIngestedBlockTime := bp.lastIngestedBlock.LastTimestamp() | |
| if time.Now().After(lastIngestedBlockTime.Add(bp.healthTimeout)) { | |
| return false, fmt.Errorf("last ingested block was %s ago", time.Since(lastIngestedBlockTime)) | |
| } | |
| return true, nil | |
| } |
| // HealthCheck checks if the last executed batch was more than healthTimeout ago | ||
| func (br *batchRegistry) HealthCheck() (bool, error) { | ||
| lastExecutedBatchTime := br.lastExecutedBatch.LastTimestamp() | ||
| if time.Now().After(lastExecutedBatchTime.Add(br.healthTimeout)) { | ||
| return false, fmt.Errorf("last executed batch was %s ago", time.Since(lastExecutedBatchTime)) | ||
| } | ||
|
|
||
| return true, nil | ||
| } |
There was a problem hiding this comment.
The HealthCheck method correctly assesses the health based on the time elapsed since the last executed batch. Consider adding a comment explaining what constitutes a healthy or unhealthy state, similar to the suggestion made in the block_processor.go file.
+ // HealthCheck determines if the registry is healthy based on the time since the last batch execution.Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // HealthCheck checks if the last executed batch was more than healthTimeout ago | |
| func (br *batchRegistry) HealthCheck() (bool, error) { | |
| lastExecutedBatchTime := br.lastExecutedBatch.LastTimestamp() | |
| if time.Now().After(lastExecutedBatchTime.Add(br.healthTimeout)) { | |
| return false, fmt.Errorf("last executed batch was %s ago", time.Since(lastExecutedBatchTime)) | |
| } | |
| return true, nil | |
| } | |
| // HealthCheck determines if the registry is healthy based on the time since the last batch execution. | |
| // HealthCheck checks if the last executed batch was more than healthTimeout ago | |
| func (br *batchRegistry) HealthCheck() (bool, error) { | |
| lastExecutedBatchTime := br.lastExecutedBatch.LastTimestamp() | |
| if time.Now().After(lastExecutedBatchTime.Add(br.healthTimeout)) { | |
| return false, fmt.Errorf("last executed batch was %s ago", time.Since(lastExecutedBatchTime)) | |
| } | |
| return true, nil | |
| } |
Why this change is needed
https://github.com/ten-protocol/ten-internal/issues/2631
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks