feat(da): metrics #11
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughThe changes in this pull request include updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit can generate a title for your PR based on the changes.Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
tuxcanfly
force-pushed
the
tux/da-metrics
branch
2 times, most recently
from
0d453cf
to
b24b6cc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
sequencing/metrics.go (2)
16-33: LGTM: Metrics struct provides a comprehensive set of initial metrics.The
Metricsstruct aligns well with the PR objectives, providing valuable insights for rollup developers. It includes metrics for gas price, blob size, transaction status, pending blocks, and included block height.I noticed TODO comments for potential future metrics (gas used, wallet balance, cost per byte). Would you like me to create GitHub issues to track these enhancements?
1-86: Great implementation of the metrics system!The
metrics.gofile provides a comprehensive and well-structured metrics system for the sequencer, aligning perfectly with the PR objectives. The implementation follows Prometheus best practices and offers flexibility through custom labeling.Next steps to consider:
- Implement the additional metrics mentioned in the TODO comments (gas used, wallet balance, cost per byte).
- Create unit tests for the
PrometheusMetricsandNopMetricsfunctions to ensure their correctness.- Document how these metrics should be used and interpreted by rollup developers.
Overall, this is a solid foundation for the metrics system that will greatly benefit rollup developers in making informed decisions.
go.mod (1)
8-11: Consider updating go-kit to the latest version.The addition of
github.com/go-kit/kitandgithub.com/prometheus/client_golangas direct dependencies aligns well with the PR objectives for implementing metrics. However,go-kitis currently set to version v0.13.0, while the latest version is v0.14.0.Consider updating
github.com/go-kit/kitto v0.14.0 to ensure you have the latest features and bug fixes:- github.com/go-kit/kit v0.13.0 + github.com/go-kit/kit v0.14.0The
github.com/prometheus/client_golang v1.19.1dependency is up-to-date and appropriate for implementing Prometheus metrics.da/da.go (1)
197-197: LGTM with suggestion: Update to SubmitBatch methodThe population of the
BlobSizefield in theSubmitBatchmethod is consistent with its addition to theBaseResultstruct. This provides valuable information about the total size of the submitted batch, which aligns with the PR objectives.However, consider adding a check for potential overflow when calculating
blobSize. Although unlikely, if the total size of all blobs exceeds the maximum value of uint64, it could lead to unexpected behavior.Consider adding an overflow check, for example:
if blobSize+uint64(len(blob)) < blobSize { // Handle overflow message = "Blob size overflow detected" log.Warn(message) break }sequencing/sequencer.go (3)
126-127: LGTM! Consider initializing the metrics field.The addition of the
metricsfield to theSequencerstruct is a good way to incorporate metrics tracking. However, ensure that this field is properly initialized in theNewSequencerfunction to avoid potential nil pointer dereferences.Consider updating the
NewSequencerfunction to initialize themetricsfield:func NewSequencer(daAddress, daAuthToken string, daNamespace []byte, batchTime time.Duration) (*Sequencer, error) { // ... existing code ... s := &Sequencer{ // ... existing fields ... + metrics: NewMetrics(), // Assuming there's a NewMetrics() function } // ... rest of the function ... }
187-193: LGTM! Consider adding error handling for metric updates.The
recordMetricsmethod is well-structured and updates the relevant metrics. However, some metrics libraries'Setmethods can return errors, which are currently not being handled.Consider adding error handling for each metric update:
func (c *Sequencer) recordMetrics(gasPrice float64, blobSize uint64, status da.StatusCode, numPendingBlocks int, includedBlockHeight uint64) { - c.metrics.GasPrice.Set(float64(gasPrice)) - c.metrics.LastBlobSize.Set(float64(blobSize)) - c.metrics.TransactionStatus.Set(float64(status)) - c.metrics.NumPendingBlocks.Set(float64(numPendingBlocks)) - c.metrics.IncludedBlockHeight.Set(float64(includedBlockHeight)) + if err := c.metrics.GasPrice.Set(float64(gasPrice)); err != nil { + log.Error("Failed to set GasPrice metric", "error", err) + } + // Repeat for other metrics... }This change will help catch and log any errors that occur during metric updates, aiding in debugging and monitoring.
256-257: LGTM! Consider consistent error handling.The addition of the
recordMetricscall in thesubmitBatchToDAmethod is well-placed to capture metrics after each submission attempt. This aligns well with the PR objectives of introducing metrics for DA submission data.For consistency with other error handling in this method, consider wrapping the
recordMetricscall in an error check:- c.recordMetrics(gasPrice, res.BlobSize, res.Code, len(batchesToSubmit), res.DAHeight) + if err := c.recordMetrics(gasPrice, res.BlobSize, res.Code, len(batchesToSubmit), res.DAHeight); err != nil { + log.Error("Failed to record metrics", "error", err) + } attempt += 1This change assumes that
recordMetricsis updated to return an error as suggested in the previous comment. It maintains consistency with the error handling approach used elsewhere in this method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- da/da.go (2 hunks)
- go.mod (1 hunks)
- sequencing/metrics.go (1 hunks)
- sequencing/sequencer.go (3 hunks)
🔇 Additional comments (7)
sequencing/metrics.go (4)
1-8: LGTM: Package declaration and imports are appropriate.The package name
sequencingis suitable for the context, and the imported libraries are relevant for implementing metrics using Prometheus.
10-14: LGTM: Constant definition is appropriate.The
MetricsSubsystemconstant provides a consistent label for all metrics in this package, which is a good practice for organizing and identifying metrics.
35-75: LGTM: PrometheusMetrics function is well-implemented.The
PrometheusMetricsfunction provides a flexible and robust way to initialize Prometheus metrics. It allows for custom labels and follows Prometheus best practices. The metric definitions are clear and descriptive, which will aid in understanding and using these metrics.
77-86: LGTM: NopMetrics function provides a useful no-op implementation.The
NopMetricsfunction is a valuable addition, providing a no-op implementation of theMetricsstruct. This is particularly useful for testing scenarios or when metrics collection is not needed, ensuring that theMetricsinterface can be used without side effects.da/da.go (2)
83-84: LGTM: Addition of BlobSize fieldThe addition of the
BlobSizefield to theBaseResultstruct is a good enhancement. It aligns with the PR objectives to provide more metrics for rollup developers, specifically related to Data Availability (DA) submission data. The field type (uint64) is appropriate for storing blob sizes.
Line range hint
1-270: Overall assessment: Well-implemented featureThe changes in this file successfully implement the addition of a blob size metric for DA submissions. This aligns well with the PR objectives to provide more metrics for rollup developers, specifically related to Data Availability (DA) submission data.
The implementation is clean, minimal, and well-integrated into the existing code structure. It adds valuable information that will help rollup developers make more informed decisions, as outlined in the linked issue #1126.
Great job on this implementation!
sequencing/sequencer.go (1)
Line range hint
1-356: Overall assessment: Changes align well with PR objectives.The introduced changes successfully implement metrics reporting for DA submission data, addressing the objectives outlined in the PR summary and linked issue #1126. The metrics cover key aspects such as gas price, blob size, transaction status, pending blocks, and included block height, which will provide valuable insights for rollup developers.
A few minor suggestions have been made to enhance error handling and ensure proper initialization of the metrics field. These improvements will contribute to the robustness and reliability of the metrics reporting system.
To fully meet the objectives from the linked issue, consider the following next steps:
- Implement a basic metrics endpoint for exposing these metrics.
- Add metrics for DA cost or burn rate and current wallet balance of the DA batch signer.
- Include metrics for the latest DA bundles submitted and their success rates.
These additional metrics will further empower rollup developers to make informed decisions based on comprehensive data.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
main.go (1)
Line range hint
1-95: Overall assessment: Good implementation with a minor improvement needed.The changes successfully introduce Prometheus metrics functionality as per the PR objectives. The implementation is well-integrated into the existing code structure. However, there's a minor issue with the order of operations in the metrics server setup that should be addressed.
Key points:
- Metrics-related imports, variables, and flags are correctly added.
- The metrics server is set up in a non-blocking manner using a goroutine.
- The NewSequencer call is properly updated to include the metrics parameter.
Please address the suggested reordering of operations in the metrics server setup to ensure the /metrics endpoint is always available when the server starts.
sequencing/metrics.go (2)
58-88: Review metric names and help descriptions for clarity and consistencyThe names and help descriptions of the metrics should be clear and consistently formatted to enhance usability in monitoring tools. For example:
- The
GasPricemetric's help text is "The gas price of DA." Consider specifying the unit (e.g., wei, gwei) for clarity.- The
IncludedBlockHeightmetric's name could be more descriptive, such aslast_included_block_height, to clearly indicate its purpose.
91-100: EnsureNopMetricscovers all metrics for consistencyCurrently, the
NopMetricsfunction initializes all metrics except forCostPerByteandWalletBalance, which are commented out in theMetricsstruct. Once you decide to implement or remove those metrics, ensure thatNopMetricsis updated accordingly to maintain consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- main.go (4 hunks)
- sequencing/metrics.go (1 hunks)
- sequencing/sequencer.go (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sequencing/sequencer.go
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
sequencing/metrics.go
[failure] 1-1:
: # github.com/rollkit/centralized-sequencer/sequencing [github.com/rollkit/centralized-sequencer/sequencing.test]
🔇 Additional comments (5)
main.go (4)
15-15: LGTM: Prometheus HTTP handler import added.The addition of the Prometheus HTTP handler import is correct and necessary for the new metrics functionality.
36-37: LGTM: Metrics control variables added.The addition of
metricsEnabledandmetricsAddressvariables is appropriate for controlling the new metrics functionality. The naming is clear and consistent with the existing style.
46-47: LGTM: Metrics flags added with appropriate defaults.The new flags for enabling metrics and setting the metrics address are well-defined. The default values (metrics disabled, address ":8080") are reasonable, and the descriptions are clear.
78-79: LGTM: NewSequencer call updated with metrics parameter.The NewSequencer function call has been correctly updated to include the metrics parameter. The use of DefaultMetricsProvider with the metricsEnabled flag and properly encoded namespace is appropriate.
sequencing/metrics.go (1)
4-4:⚠️ Potential issueRemove unused import of
github.com/go-kit/kit/metricsThe import statement on line 4 for
github.com/go-kit/kit/metricsappears to be unused directly in this file. Since you are referencingmetrics.Gaugethrough the specific packages (prometheusanddiscard), you can remove this import to clean up the code.Verify if the
metricspackage is used elsewhere in the code:If there are no direct references to
metrics.in the files, the import can be safely removed.
tuxcanfly
force-pushed
the
tux/da-metrics
branch
from
2abb64b
to
0847d53
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
sequencing/metrics.go (2)
10-17: LGTM: Constants and type definitions are well-defined.The
MetricsSubsystemconstant andMetricsProvidertype are appropriately defined. The function type forMetricsProviderallows for flexible metrics creation based on the chain ID.Consider adding a brief comment explaining the purpose of the
MetricsProvidertype, similar to the comment forMetricsSubsystem. This would enhance code readability and documentation.
1-100: Overall implementation is good, but some additions are needed to fully meet PR objectives.The
metrics.gofile implements a well-structured metrics system for the sequencer, providing both Prometheus-based and no-op metrics. The implementation is flexible and allows for easy enabling/disabling of metrics collection.However, to fully meet the PR objectives and address the requirements from issue #1126, consider the following improvements:
Implement the missing metrics:
- DA cost or burn rate
- Current wallet balance of the DA batch signer
- Details about the latest DA bundles submitted, including success rates
Add functionality to expose these metrics in a format suitable for visualization in a dashboard like Grafana.
Ensure that all implemented metrics provide valuable insights for rollup developers to make informed decisions about their rollups.
To implement these improvements:
- Add new fields to the
Metricsstruct for the missing metrics.- Implement methods to update these metrics based on the sequencer's operations.
- Consider adding a method or separate function to export all metrics in a format suitable for Grafana or similar dashboarding tools.
Would you like assistance in implementing these additional features to fully meet the PR objectives?
🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 1-1:
: # github.com/rollkit/centralized-sequencer/sequencing [github.com/rollkit/centralized-sequencer/sequencing.test]sequencing/sequencer.go (4)
127-129: LGTM! Consider adding comments for new fields.The addition of
metricsProviderandmetricsfields to theSequencerstruct aligns well with the PR objectives for implementing metrics reporting. This change provides a foundation for collecting and managing metrics data.Consider adding brief comments to explain the purpose of these new fields, especially for
metricsProvider, to improve code readability:type Sequencer struct { // ... existing fields ... + // metricsProvider creates metrics instances for specific rollup IDs metricsProvider MetricsProvider + // metrics holds the current metrics data for the sequencer metrics *Metrics }
152-153: LGTM! Consider clarifying the metrics initialization process.The initialization of
metricsProviderandmetricsfields in theNewSequencerfunction is appropriate. UsingNopMetrics()as an initial value formetricsis a good practice, providing a no-op implementation until the actual metrics are set up.To improve clarity, consider adding a brief comment explaining why
metricsis initialized withNopMetrics()and will be properly set later:metricsProvider: metricsProvider, - metrics: NopMetrics(), // Initialized from metricsProvider in SubmitRollupTransaction + metrics: NopMetrics(), // Temporary no-op metrics; will be initialized with actual metrics in SubmitRollupTransaction
191-197: LGTM! Consider adding error handling for metric recording.The
recordMetricsmethod is a great addition that aligns well with the PR objectives. It captures important metrics related to DA submission data, providing valuable insights for rollup developers as outlined in issue #1126.Consider adding error handling for metric recording operations. While unlikely, errors in metric recording shouldn't affect the main functionality:
func (c *Sequencer) recordMetrics(gasPrice float64, blobSize uint64, status da.StatusCode, numPendingBlocks int, includedBlockHeight uint64) { - c.metrics.GasPrice.Set(float64(gasPrice)) - c.metrics.LastBlobSize.Set(float64(blobSize)) - c.metrics.TransactionStatus.Set(float64(status)) - c.metrics.NumPendingBlocks.Set(float64(numPendingBlocks)) - c.metrics.IncludedBlockHeight.Set(float64(includedBlockHeight)) + if err := c.metrics.GasPrice.Set(float64(gasPrice)); err != nil { + log.Errorf("Failed to record GasPrice metric: %v", err) + } + // Repeat for other metrics... }
304-305: LGTM! Consider adding error handling for metrics initialization.The initialization of the
metricsfield using themetricsProviderwhen therollupIdis first set is a good approach. This ensures that metrics are properly initialized for each specific rollup ID, which is particularly useful in multi-rollup scenarios.Consider adding error handling for the metrics initialization:
if c.rollupId == nil { c.rollupId = rollupId - c.metrics = c.metricsProvider(hex.EncodeToString(rollupId)) + metrics, err := c.metricsProvider(hex.EncodeToString(rollupId)) + if err != nil { + log.Errorf("Failed to initialize metrics for rollup ID %s: %v", hex.EncodeToString(rollupId), err) + // Consider using NopMetrics() as a fallback + c.metrics = NopMetrics() + } else { + c.metrics = metrics + } } else { // ... existing code ... }This change ensures that the system can continue to function even if there's an issue with metrics initialization, while also logging the error for debugging purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- da/da.go (2 hunks)
- go.mod (1 hunks)
- main.go (4 hunks)
- sequencing/metrics.go (1 hunks)
- sequencing/sequencer.go (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- da/da.go
- go.mod
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
sequencing/metrics.go
[failure] 1-1:
: # github.com/rollkit/centralized-sequencer/sequencing [github.com/rollkit/centralized-sequencer/sequencing.test]
🔇 Additional comments (14)
main.go (6)
15-15: LGTM: Prometheus import added correctly.The new import for Prometheus HTTP handler is correctly added and necessary for the metrics functionality.
36-37: LGTM: Metrics control variables added.The new variables
metricsEnabledandmetricsAddressare correctly added to control the metrics functionality.
46-47: LGTM: Metrics flags added with appropriate defaults.The new flags for enabling metrics and setting the metrics address are correctly added with sensible default values.
67-76: LGTM: Metrics server setup is correct and addresses past concerns.The metrics server setup looks good:
- It's correctly placed in a separate goroutine.
- The HTTP handler is set up before starting the server, addressing the potential race condition mentioned in the past review.
- Proper error handling is in place.
Line range hint
1-95: Overall: Excellent implementation of metrics functionality.The changes in this file successfully implement the metrics reporting for DA submission data as outlined in the PR objectives. The new functionality is well-integrated into the existing code structure, with appropriate error handling and user controls. The implementation addresses the requirements specified in the linked issue #1126, providing a foundation for exposing metrics that will assist rollup developers in making informed decisions.
Some key points:
- Metrics can be enabled/disabled via a command-line flag.
- A separate metrics server is set up when enabled.
- The sequencer is updated to use the new metrics functionality.
These changes lay the groundwork for further metrics implementation and dashboard integration as specified in the linked issue.
78-79: LGTM: Sequencer creation updated with metrics.The changes to sequencer creation look good:
DefaultMetricsProvideris used correctly with themetricsEnabledflag and namespace.- The
NewSequencerfunction call is updated to include the newmetricsparameter.These changes align well with the PR objectives of adding metrics functionality.
Let's verify the
NewSequencerfunction signature change:sequencing/metrics.go (5)
1-8: LGTM: Package declaration and imports are appropriate.The package name
sequencingand the imported libraries are well-suited for implementing metrics in the sequencer. The use of go-kit metrics and Prometheus client is a good choice for flexible and standardized metrics implementation.🧰 Tools
🪛 GitHub Check: lint / golangci-lint
[failure] 1-1:
: # github.com/rollkit/centralized-sequencer/sequencing [github.com/rollkit/centralized-sequencer/sequencing.test]
19-28: LGTM: DefaultMetricsProvider function is well-implemented.The
DefaultMetricsProviderfunction is a well-designed factory that returns either Prometheus-based metrics or no-op metrics based on theenabledflag. This implementation aligns with the PR objective of adding a basic metrics endpoint and provides flexibility for enabling/disabling metrics as needed.
91-100: LGTM: NopMetrics function is correctly implemented.The
NopMetricsfunction properly initializes all metrics defined in theMetricsstruct with no-op gauges usingdiscard.NewGauge(). This implementation provides an efficient alternative when metrics collection is disabled, aligning with the flexible metrics system design.
30-47:⚠️ Potential issueAddress TODO and implement missing metrics.
The
Metricsstruct partially addresses the PR objectives by including metrics for gas price, blob size, transaction status, pending blocks, and included block height. However, there are still some issues to address:
- The TODO comment and commented-out metrics (
CostPerByteandWalletBalance) should be implemented to fully meet the PR objectives.- Consider adding a metric for DA cost or burn rate, as mentioned in the linked issue #1126.
To resolve these issues:
- Implement the
CostPerByteandWalletBalancemetrics.- Add a new metric for DA cost or burn rate.
- Remove the TODO comment once the implementation is complete.
Would you like assistance in implementing these missing metrics?
49-89:⚠️ Potential issueImprove error handling for label-value pairs in PrometheusMetrics.
The
PrometheusMetricsfunction correctly implements the required metrics using Prometheus. However, there's a potential issue in the processing of label-value pairs:The loop in lines 54-56 assumes that
labelsAndValuescontains pairs of labels and values. If an odd number of elements is passed, it could lead to a runtime panic when accessing out-of-range indices.To address this:
- Add a check at the beginning of the function to ensure
labelsAndValueshas an even length.- If the length is odd, either log an error or panic with a descriptive message.
Here's a suggested implementation:
func PrometheusMetrics(namespace string, labelsAndValues ...string) *Metrics { if len(labelsAndValues)%2 != 0 { // Option 1: Log an error and return NopMetrics // log.Error("Odd number of labelsAndValues provided to PrometheusMetrics") // return NopMetrics() // Option 2: Panic with a descriptive message panic("PrometheusMetrics requires an even number of labelsAndValues") } // Rest of the function remains the same ... }This change will prevent potential runtime errors and make the function more robust.
sequencing/sequencer.go (3)
133-133: LGTM! Function signature update is appropriate.The addition of the
metricsProviderparameter to theNewSequencerfunction signature is a good change. It allows theSequencerto be initialized with a metrics provider, ensuring that metrics can be collected from the moment theSequenceris created. This change is consistent with the new fields added to theSequencerstruct and supports the PR objectives for implementing metrics reporting.
260-260: LGTM! Metrics recording is well-placed.The addition of the
c.recordMetricscall at the end of each iteration in the DA submit retry loop is an excellent change. This ensures that metrics are recorded after each submission attempt, regardless of the outcome, providing valuable data for analyzing DA submission performance. The placement captures the latest state after each attempt, which aligns well with the PR objectives of implementing comprehensive metrics reporting.
Line range hint
1-353: Overall assessment: Changes align well with PR objectivesThe modifications to
sequencing/sequencer.gosuccessfully implement metrics collection and reporting for DA submission data, aligning closely with the PR objectives and addressing the requirements outlined in issue #1126. Key improvements include:
- Addition of
metricsProviderandmetricsfields to theSequencerstruct.- Update of the
NewSequencerfunction to accept ametricsProvider.- Implementation of the
recordMetricsmethod to capture various DA-related metrics.- Integration of metrics recording in the
submitBatchToDAmethod.- Proper initialization of metrics in the
SubmitRollupTransactionmethod.These changes provide valuable insights for rollup developers, enhancing their ability to make informed decisions about their rollups. The implementation is generally well-structured and maintains good coding practices.
To further improve the code:
- Consider adding comments to new fields in the
Sequencerstruct for better documentation.- Implement error handling for metric recording and initialization operations to ensure robustness.
- Clarify the metrics initialization process in the
NewSequencerfunction with a more descriptive comment.These minor enhancements will contribute to better code maintainability and reliability.
tuxcanfly
force-pushed
the
tux/da-metrics
branch
from
0847d53
to
b54ce8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
main.go (1)
79-80: LGTM: Metrics integration with NewSequencer, but TODO needs addressing.The integration of metrics with the NewSequencer function is correct and aligns with the changes in the sequencing package. However, there's a TODO comment that needs to be addressed:
metrics := sequencing.DefaultMetricsProvider(metricsEnabled)("") // TODO use rollupId as chain_idPlease replace the empty string with the appropriate rollupId when it's available. This will ensure that the metrics are properly associated with the specific rollup chain.
sequencing/sequencer_test.go (3)
54-54: LGTM! Consider adding a comment for clarity.The addition of
NopMetrics()as the last parameter toNewSequenceris correct and aligns with the updated method signature. This change allows for metrics functionality in the Sequencer, which is beneficial for monitoring and debugging.Consider adding a brief comment explaining the purpose of
NopMetrics()for better code readability:- seq, err := NewSequencer(MockDAAddressHTTP, "authToken", []byte("namespace"), 10*time.Second, NopMetrics()) + // Use NopMetrics() for testing as we don't need actual metrics collection + seq, err := NewSequencer(MockDAAddressHTTP, "authToken", []byte("namespace"), 10*time.Second, NopMetrics())
66-66: LGTM! Consider consistent formatting.The addition of
NopMetrics()as the last parameter toNewSequenceris correct and consistent with the previous change.For consistency with the previous test function, consider adding a similar comment explaining the use of
NopMetrics():- seq, err := NewSequencer(MockDAAddressHTTP, "authToken", []byte("namespace"), 10*time.Second, NopMetrics()) + // Use NopMetrics() for testing as we don't need actual metrics collection + seq, err := NewSequencer(MockDAAddressHTTP, "authToken", []byte("namespace"), 10*time.Second, NopMetrics())
Line range hint
1-186: Consider expanding metrics implementation to meet PR objectives.The changes in this file successfully introduce metrics functionality to the Sequencer in test cases, which is a step towards meeting the PR objectives. However, to fully address the goals outlined in the PR summary and linked issue #1126, consider the following suggestions:
- Implement actual metrics collection in the main Sequencer implementation.
- Add specific metrics for DA submission data, such as DA cost, burn rate, and latest DA bundle details.
- Create new test cases that verify the correct collection and reporting of these metrics.
- Ensure that the metrics can be easily integrated with visualization tools like Grafana, as mentioned in the objectives.
Would you like assistance in planning or implementing these additional features to fully meet the PR objectives?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- da/da.go (2 hunks)
- go.mod (1 hunks)
- main.go (4 hunks)
- sequencing/metrics.go (1 hunks)
- sequencing/sequencer.go (4 hunks)
- sequencing/sequencer_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- da/da.go
- go.mod
- sequencing/metrics.go
- sequencing/sequencer.go
🧰 Additional context used
🔇 Additional comments (4)
main.go (4)
15-16: LGTM: Prometheus metrics import added.The addition of the Prometheus HTTP handler import is appropriate for implementing the metrics functionality as outlined in the PR objectives.
37-38: LGTM: Metrics configuration variables added.The addition of
metricsEnabledandmetricsAddressvariables provides the necessary configuration options for the metrics functionality, aligning with the PR objectives.
47-48: LGTM: Metrics configuration flags added.The addition of flags for
metricsEnabledandmetricsAddressprovides appropriate command-line configuration options for the metrics functionality. The descriptions are clear and the default values are sensible.
68-77: LGTM: Metrics server setup implemented correctly.The metrics server setup is well-implemented:
- It runs in a separate goroutine to avoid blocking the main thread.
- Error handling is appropriate.
- The metrics endpoint is correctly set up using the Prometheus HTTP handler.
- The order of operations has been corrected as per the previous review comment.
This implementation aligns with the PR objectives of exposing metrics for rollup developers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
README.md (1)
57-58: LGTM! Consider adding a note about metrics usage.The new flags for metrics are well-documented and consistent with the existing table. They align with the PR objectives of adding metrics functionality.
To provide more context for users, consider adding a brief note after the table explaining how to access the metrics once enabled. For example:
Note: When metrics are enabled, they can be accessed at the specified metrics-address (e.g., http://localhost:8080/metrics).
tuxcanfly
force-pushed
the
tux/da-metrics
branch
from
c2faa60
to
5de62cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
main.go (3)
49-50: Consider enhancing metrics configuration.While the flag definitions are functional, consider these improvements:
- Add validation for the metrics address format
- Document potential port conflicts in the help text
- Consider using a less common default port to avoid conflicts
- flag.StringVar(&metricsAddress, "metrics-address", ":8080", "Address to expose Prometheus metrics") + flag.StringVar(&metricsAddress, "metrics-address", ":9090", "Address to expose Prometheus metrics (format: [host]:port)")
81-82: Document metrics implementation details.The metrics integration lacks documentation about:
- The purpose of the empty string parameter in
metrics("")- What metrics are being collected
- How these metrics align with the PR objectives (DA submission data)
Consider adding comments explaining the metrics implementation and its relationship to the DA metrics requirements from issue #1126.
81-82: Consider architectural improvements for observability.To enhance the observability stack:
- Add a health check endpoint alongside metrics
- Implement proper shutdown handling for both gRPC and metrics servers
- Consider adding a readiness probe
- Document the metrics format and dashboard setup (e.g., Grafana)
This would provide a more complete observability solution for rollup developers.
sequencing/metrics.go (1)
1-95: Consider adding more rollup-specific metrics.While the current implementation provides a good foundation for basic DA metrics, consider adding more rollup-specific metrics that would help developers make informed decisions, such as:
- Average DA submission latency
- DA submission success rate over time
- DA cost trends
- Bundle size distribution
These metrics would provide valuable insights for rollup developers as mentioned in issue #1126.
README.md (1)
57-58: Enhance metrics documentation while the changes look good.The new flags are correctly documented and align with the PR objectives. However, to better serve rollup developers (as mentioned in issue #1126), consider enhancing the documentation with:
- A new section describing the available metrics and their purposes
- Examples of querying these metrics
- Basic setup instructions for visualizing these metrics in Grafana
Example addition:
| `metrics-address` |address to expose prometheus metrics|`":8080"`| <!-- markdownlint-enable MD013 --> + +## Metrics + +When enabled with the `metrics` flag, the following Prometheus metrics are exposed at `http://<metrics-address>/metrics`: + +| Metric Name | Type | Description | +|------------|------|-------------| +| da_submission_size_bytes | Gauge | Size of DA submissions in bytes | +| da_submission_cost | Gauge | Cost of DA submissions | +| da_wallet_balance | Gauge | Current wallet balance of the DA batch signer | + +### Visualizing Metrics + +To visualize these metrics in Grafana: + +1. Configure Prometheus to scrape metrics from the sequencer's metrics endpoint +2. Import the [example dashboard](./grafana/dashboards/da-metrics.json)go.mod (1)
9-9: Consider upgrading go-kit to the latest version.While
v0.13.0is stable, consider upgrading to the latest version (v0.14.0) for potential improvements and bug fixes.- github.com/go-kit/kit v0.13.0 + github.com/go-kit/kit v0.14.0sequencing/sequencer_test.go (1)
70-70: Consider replacing sleep with proper synchronization.While the addition of
NopMetrics()is good, the test relies on a fixed sleep duration which could be flaky. Consider using a more reliable synchronization mechanism like channels or polling with timeout to verify transaction processing.Example approach:
// Instead of sleep, poll until the transaction appears or timeout ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() for { nextBatchresp, err := seq.GetNextBatch(ctx, sequencing.GetNextBatchRequest{RollupId: rollupId, LastBatchHash: nil}) if err != nil { t.Fatal(err) } if len(nextBatchresp.Batch.Transactions) > 0 { break } select { case <-ctx.Done(): t.Fatal("timeout waiting for transaction") case <-time.After(100 * time.Millisecond): continue } }Also applies to: 80-81
sequencing/sequencer.go (2)
497-505: Optimize metric recording by handling nil checks withinMetricsmethodsCurrently, there's a nil check for
c.metricsbefore setting each metric. If theMetricsmethods already handle nil checks internally, this external check may be redundant.Consider simplifying the code by removing the outer nil check if it's safe to do so.
-func (c *Sequencer) recordMetrics(gasPrice float64, blobSize uint64, status da.StatusCode, numPendingBlocks int, includedBlockHeight uint64) { - if c.metrics != nil { - c.metrics.GasPrice.Set(float64(gasPrice)) - c.metrics.LastBlobSize.Set(float64(blobSize)) - c.metrics.TransactionStatus.Set(float64(status)) - c.metrics.NumPendingBlocks.Set(float64(numPendingBlocks)) - c.metrics.IncludedBlockHeight.Set(float64(includedBlockHeight)) - } +func (c *Sequencer) recordMetrics(gasPrice float64, blobSize uint64, status da.StatusCode, numPendingBlocks int, includedBlockHeight uint64) { + c.metrics.GasPrice.Set(float64(gasPrice)) + c.metrics.LastBlobSize.Set(float64(blobSize)) + c.metrics.TransactionStatus.Set(float64(status)) + c.metrics.NumPendingBlocks.Set(float64(numPendingBlocks)) + c.metrics.IncludedBlockHeight.Set(float64(includedBlockHeight)) }
568-568: EnsurerecordMetricsis called only when metrics are enabledBefore calling
c.recordMetrics(...), verify that metrics are enabled to prevent potential nil pointer exceptions ifc.metricsis nil.Add a nil check or ensure that
c.metricsis always initialized when metrics are enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- README.md (1 hunks)
- da/da.go (2 hunks)
- go.mod (1 hunks)
- main.go (4 hunks)
- sequencing/metrics.go (1 hunks)
- sequencing/sequencer.go (5 hunks)
- sequencing/sequencer_test.go (2 hunks)
🧰 Additional context used
🪛 golangci-lint
sequencing/sequencer.go
285-285: File is not
goimports-ed with -local github.com/rollkit(goimports)
🔇 Additional comments (11)
main.go (3)
9-9: LGTM: Import statements are appropriate.The new imports are necessary for the metrics functionality and correctly placed.
Also applies to: 15-16
38-39: LGTM: Configuration variables are well-defined.The new metrics configuration variables follow Go naming conventions and use appropriate types.
70-79: Enhance metrics server security and shutdown handling.While the basic setup is functional, consider these security and reliability improvements:
- The metrics endpoint is exposed without authentication or TLS
- The server lacks graceful shutdown handling
- The
#nosec G114comment bypasses a security check without documentationConsider implementing:
- Basic authentication for the metrics endpoint
- TLS configuration
- Graceful shutdown using context
- Document the security implications in comments
sequencing/metrics.go (5)
1-14: LGTM! Well-structured metrics setup.Good choice using go-kit metrics as an abstraction layer, which allows for flexibility in metric backends while maintaining a clean interface.
16-28: LGTM! Clean factory pattern implementation.The MetricsProvider design effectively supports per-chain metrics configuration while maintaining clean separation of concerns.
42-42: Consider using Counter instead of Gauge for TransactionStatus.Since transaction status represents discrete events (success/failure), using a Counter would be more appropriate than a Gauge. This would allow tracking the rate of successful vs failed transactions over time.
52-56: Add validation for label-value pairs.The current implementation could panic if an odd number of label-value pairs is provided. Add validation at the start of the function.
func PrometheusMetrics(labelsAndValues ...string) *Metrics { + if len(labelsAndValues)%2 != 0 { + panic("metrics: uneven number of label values") + } labels := []string{} for i := 0; i < len(labelsAndValues); i += 2 {
86-95: LGTM! Clean no-op implementation.The NopMetrics implementation correctly provides no-op versions of all metrics, following the null object pattern.
go.mod (1)
12-12: LGTM on Prometheus client dependency!The addition of
github.com/prometheus/client_golang v1.19.1aligns well with the PR objectives for implementing metrics reporting. This is a recent and stable version of the Prometheus client library.sequencing/sequencer_test.go (1)
54-54: Verify the timeout increase rationale.While the addition of
NopMetrics()aligns well with the metrics functionality, the timeout increase from 1s to 10s seems significant for a constructor test. Could you explain why this increase was necessary?da/da.go (1)
83-84: LGTM! The BlobSize field addition is well-structured.The new field is appropriately typed and documented, aligning with the PR's metrics reporting objectives.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
main.go (4)
38-39: LGTM: Well-structured metrics configuration.The metrics configuration flags are well-defined with appropriate defaults. However, consider adding a note in the documentation about securing the metrics endpoint in production environments, as it may expose sensitive information.
Consider implementing authentication or network-level controls for the metrics endpoint in production deployments.
Also applies to: 49-50
73-73: Document security implications of #nosec.The
#nosec G114directive bypasses the security check for non-TLS HTTP usage. Please document why TLS is not required in this context.Add a comment explaining the security implications and why it's safe to bypass TLS for metrics in this case.
81-82: Document metrics provider parameter usage.The empty string passed to
metrics("")needs documentation explaining its purpose.Add a comment explaining what this parameter represents and why an empty string is appropriate here.
Line range hint
38-82: Consider adding metrics documentation.While the implementation aligns with the PR objectives, consider adding documentation that:
- Lists the available metrics and their meanings
- Provides example Grafana queries/dashboards
- Explains the metrics data retention policy
This would help rollup developers better utilize these metrics for decision-making, as outlined in issue #1126.
sequencing/metrics.go (2)
36-40: Consider removing or addressing the TODO commentsThe TODOs for gas used and wallet balance metrics indicate pending work. If these metrics are not planned for immediate implementation, consider removing the comments to keep the codebase clean. Alternatively, if you plan to implement them soon, I can assist with adding these metrics.
Would you like help in implementing these metrics or opening a GitHub issue to track this task?
62-86: Enhance metricHelpdescriptions for clarityConsider providing more detailed descriptions for the metrics to improve observability and maintainability. Clear
Helptext assists in understanding the purpose and usage of each metric.Apply this diff to enhance the descriptions:
Help: "The gas price of DA.", +// Example: "The current gas price used for Data Availability transactions." Help: "The size in bytes of the last DA blob.", +// Example: "Size in bytes of the most recently submitted Data Availability blob." Help: "The last DA included block height.", +// Example: "The block height at which the last Data Availability submission was included."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- main.go (4 hunks)
- sequencing/metrics.go (1 hunks)
- sequencing/sequencer.go (5 hunks)
🔇 Additional comments (8)
main.go (2)
9-9: LGTM: Required imports for metrics functionality.The new imports are correctly organized and necessary for the Prometheus metrics implementation.
Also applies to: 15-15
70-79: Fix race condition in metrics server setup.The handler setup after ListenAndServe can cause a race condition. Please refer to the previous review comment about reordering these operations.
sequencing/metrics.go (1)
91-99: Good use of no-op metrics for disabled metrics collectionThe implementation of
NopMetricsprovides a clean way to disable metrics collection without altering the main code logic.sequencing/sequencer.go (5)
285-286: InitializemetricsProviderto prevent nil pointer dereferenceThe
metricsProviderfield is added to theSequencerstruct but is not initialized in theNewSequencerconstructor. Using it without initialization can cause a runtime error whenc.metricsProvideris called.
309-318: ConsolidateSequencerinitialization to avoid overwriting fieldsThe
Sequencerinstancesis initialized twice in the constructor. The second initialization at line 324 overwrites the first, potentially omitting fields likemetricsandmetricsProvider. This can lead to uninitialized fields and unexpected behavior.
497-506: Good Implementation ofrecordMetricsFunctionThe
recordMetricsmethod is well-implemented, checking fornilmetrics before updating. It accurately records gas price, blob size, transaction status, number of pending blocks, and included block height.
568-568: Recording Metrics During Batch SubmissionThe call to
recordMetricsinsubmitBatchToDAensures that metrics are recorded after each submission attempt. This provides valuable insights into the batch submission process.
605-611: Check for UninitializedmetricsProviderBefore UsageIn
SubmitRollupTransaction,c.metricsProvidermight benilif not properly initialized. Using it without initialization can cause a runtime error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
main.go (2)
49-50: Consider adding address validationWhile the default metrics address is reasonable, it would be beneficial to validate the format of user-provided addresses to prevent runtime errors.
flag.StringVar(&metricsAddress, "metrics-address", ":8080", "Address to expose Prometheus metrics") +if metricsEnabled { + host, port, err := net.SplitHostPort(metricsAddress) + if err != nil { + log.Fatalf("Invalid metrics address format: %v", err) + } +}
81-82: Consider adding more DA-specific metricsBased on the PR objectives from issue #1126, consider adding these metrics:
- DA cost/burn rate
- Current wallet balance of DA batch signer
- Latest DA bundles submission stats
This would provide valuable insights for rollup developers.
sequencing/sequencer.go (2)
487-495: Add documentation for metrics recordingConsider adding documentation to explain the purpose and significance of each metric being recorded. This will help maintainers understand the metrics' importance and expected values.
Add this documentation above the function:
+// recordMetrics records various metrics related to DA submission: +// - gasPrice: The current gas price used for submission +// - blobSize: Size of the last submitted blob +// - status: Submission status code +// - numPendingBlocks: Number of blocks waiting to be submitted +// - includedBlockHeight: Height at which the submission was included func (c *Sequencer) recordMetrics(gasPrice float64, blobSize uint64, status da.StatusCode, numPendingBlocks int, includedBlockHeight uint64) {
Line range hint
673-673: Consider implementing DA verificationThe TODO comment indicates that DA verification needs to be added. Since this PR focuses on metrics, would you like me to create a separate issue to track the implementation of DA verification?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- main.go (4 hunks)
- sequencing/metrics.go (1 hunks)
- sequencing/sequencer.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sequencing/metrics.go
🔇 Additional comments (7)
main.go (4)
9-9: LGTM: Required imports added for metrics functionalityAlso applies to: 15-15
30-39: LGTM: Metrics configuration variables properly declaredVariables are well-organized and follow the existing naming conventions.
70-79: Fix metrics handler setup orderThe metrics handler setup issue identified in the previous review still needs to be addressed.
81-82: LGTM: Metrics provider properly integratedThe metrics integration looks good, but let's verify the metrics provider implementation.
✅ Verification successful
Metrics provider implementation verified and properly structured
The metrics implementation is well-designed with:
- A conditional provider that returns either Prometheus or no-op metrics based on the enabled flag
- Proper metrics structure covering essential measurements (gas price, blob size, transaction status)
- Clean separation between Prometheus and no-op implementations
- Correct namespace handling through chain_id labels
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify metrics provider implementation ast-grep --pattern 'func DefaultMetricsProvider($_) { $$$ }'Length of output: 64
Script:
#!/bin/bash # Let's try searching for DefaultMetricsProvider with ripgrep to locate the file first rg -l "DefaultMetricsProvider" # Then get the implementation details rg "DefaultMetricsProvider" -A 10Length of output: 1397
Script:
#!/bin/bash # Let's check the implementation of PrometheusMetrics and NopMetrics rg "func PrometheusMetrics|func NopMetrics" -A 5 # Also check the Metrics struct definition rg "type Metrics struct" -A 10Length of output: 1349
sequencing/sequencer.go (3)
284-285: LGTM: Metrics field additionThe metrics field is correctly defined as a pointer type, allowing for nil checks and optional metrics collection.
Line range hint
289-323: LGTM: Constructor updateThe constructor properly initializes the metrics field and maintains good error handling practices. This addresses the previous concerns about nil pointer dereference.
558-558: LGTM: Metrics recording placementThe metrics recording is correctly placed to capture data for both successful and failed submission attempts, providing valuable insights into the DA submission process.
Manav-Aggarwal
requested review from
tzdybal,
a team and
Manav-Aggarwal
and removed request for
a team
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
main.go (1)
49-50: Consider using a less common default portThe default metrics port
:8080is commonly used by other services and might lead to conflicts. Consider using a less common port (e.g.,:9090which is Prometheus's default) or documenting potential conflicts in the README.sequencing/sequencer.go (2)
Line range hint
289-323: Add nil check for metrics parameter.While the metrics integration looks good, consider adding a nil check for the metrics parameter to prevent potential panics if nil is passed. This would make the constructor more robust.
func NewSequencer(daAddress, daAuthToken string, daNamespace []byte, batchTime time.Duration, metrics *Metrics, dbPath string) (*Sequencer, error) { + if metrics == nil { + return nil, fmt.Errorf("metrics cannot be nil") + } ctx := context.Background()
487-495: Add documentation for recordMetrics method.Consider adding documentation to explain the purpose of this method and its parameters. This would improve code maintainability and help other developers understand the metrics being recorded.
+// recordMetrics records various metrics related to DA submission: +// - gasPrice: The current gas price used for submission +// - blobSize: Size of the submitted blob in bytes +// - statusCode: Status code of the DA submission attempt +// - numPendingBlocks: Number of blocks pending submission +// - includedBlockHeight: Height at which the submission was included func (c *Sequencer) recordMetrics(gasPrice float64, blobSize uint64, statusCode da.StatusCode, numPendingBlocks int, includedBlockHeight uint64) {sequencing/metrics.go (1)
39-41: Remove commented-out code for clarityThe
CostPerByteandWalletBalancemetrics are commented out in theMetricsstruct. If these metrics are not planned for future use, consider removing them to keep the codebase clean. If they are intended for future implementation, adding a// TODOcomment would make the intent clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- main.go (4 hunks)
- sequencing/metrics.go (1 hunks)
- sequencing/sequencer.go (4 hunks)
- sequencing/sequencer_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sequencing/sequencer_test.go
🔇 Additional comments (5)
main.go (3)
9-9: LGTM: Clean import additions and variable declarationsThe new imports and variable declarations are appropriate for implementing Prometheus metrics functionality.
Also applies to: 15-16, 38-39
70-79:⚠️ Potential issueFix metrics handler registration order
The metrics endpoint handler is registered after starting the HTTP server, which could lead to race conditions. The handler should be registered before starting the server.
Apply this fix:
if metricsEnabled { + http.Handle("/metrics", promhttp.Handler()) go func() { log.Printf("Starting metrics server on %v...\n", metricsAddress) err := http.ListenAndServe(metricsAddress, nil) // #nosec G114 if err != nil { log.Fatalf("Failed to serve metrics: %v", err) } - http.Handle("/metrics", promhttp.Handler()) }() }
81-85: Verify DA metrics implementationThe metrics infrastructure is set up correctly, but let's verify that all required DA metrics are implemented.
✅ Verification successful
Based on the search results, I can now generate the final response as the scripts have provided comprehensive information about the DA metrics implementation.
DA metrics are properly implemented
The codebase includes all essential DA-related metrics in
sequencing/metrics.go:
- Gas price tracking via
"The gas price of DA."- Blob size monitoring via
"The size in bytes of the last DA blob."- Transaction status counts for DA submissions
- Pending blocks counter for DA submission
- DA block height tracking
These metrics cover the key aspects of DA operations including costs, performance, and operational status, which aligns with the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DA-specific metrics implementation echo "Searching for DA metrics definitions..." rg -l "prometheus\." --type go # Look for specific metrics mentioned in issue #1126 echo "Checking for specific metrics implementation..." rg "DA|cost|burn|wallet|balance|bundle" --type goLength of output: 6034
sequencing/sequencer.go (2)
284-285: LGTM! Well-structured metrics field addition.The metrics field is appropriately defined as an unexported pointer field, following Go best practices for internal state management.
558-558: LGTM! Well-placed metrics recording.The metrics recording is strategically placed to capture data after each submission attempt, providing valuable insights into the DA submission process.
| metrics, err := sequencing.DefaultMetricsProvider(metricsEnabled)(da_namespace) | ||
| if err != nil { | ||
| log.Fatalf("Failed to create metrics: %v", err) | ||
| } | ||
| centralizedSeq, err := sequencing.NewSequencer(da_address, da_auth_token, namespace, batchTime, metrics, db_path) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add graceful shutdown for metrics server
While the metrics setup looks good, consider implementing graceful shutdown for the metrics server using a context or a shutdown channel. This would ensure clean termination of the metrics endpoint when the application exits.
Here's a suggested implementation:
+ var metricsServer *http.Server
if metricsEnabled {
+ mux := http.NewServeMux()
+ mux.Handle("/metrics", promhttp.Handler())
+ metricsServer = &http.Server{
+ Addr: metricsAddress,
+ Handler: mux,
+ }
go func() {
log.Printf("Starting metrics server on %v...\n", metricsAddress)
- err := http.ListenAndServe(metricsAddress, nil)
+ if err := metricsServer.ListenAndServe(); err != http.ErrServerClosed {
log.Fatalf("Failed to serve metrics: %v", err)
}
}()
}
// ... rest of the code ...
interrupt := make(chan os.Signal, 1)
signal.Notify(interrupt, os.Interrupt, syscall.SIGINT)
<-interrupt
+ if metricsServer != nil {
+ if err := metricsServer.Shutdown(context.Background()); err != nil {
+ log.Printf("Error shutting down metrics server: %v", err)
+ }
+ }
fmt.Println("\nCtrl+C pressed. Exiting...")
os.Exit(0)Committable suggestion was skipped due to low confidence.
Overview
Added metrics reporting for DA submission related data. Fixes rollkit/rollkit#1126
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes
Tests