Integrate prometheus to evm gateway #360
Conversation
- Added prometheus.yml to configure prometheus server - Added collector struct to gather metrics via it - Added api_errors_total metric as example of usage - Added api_request_duration metric as example of usage
illia-malachyn
requested review from
devbugging,
peterargue and
m-Peter
as code owners
|
Caution Review failedThe pull request is closed. WalkthroughThese changes integrate Prometheus metrics into the EVM gateway, significantly enhancing the monitoring of API errors and request response times. Key updates include modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BlockChainAPI
participant MetricsCollector
participant Prometheus
User->>BlockChainAPI: Make API Request
BlockChainAPI->>MetricsCollector: Start Request Timer
BlockChainAPI->>BlockChainAPI: Process Request
alt Request Fails
BlockChainAPI->>MetricsCollector: Report API Error
end
BlockChainAPI->>MetricsCollector: Stop Request Timer
MetricsCollector->>Prometheus: Push Metrics Data
Prometheus->>User: Provide Metrics Data
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@sideninja Hey, I'm adding this as skeleton. Should I create separate issue for each metrics/task as I did for this one or I can just mention #125 in every PR? |
|
To test this:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (1)
bootstrap/bootstrap.go (1)
34-34: Consider parsing the port number fromprometheus.ymlfor consistency.The port number 9091 is hardcoded in
bootstrap/bootstrap.gobut is also specified inmetrics/prometheus.yml. To align with configuration management practices, it is recommended to parse the port number from theprometheus.ymlfile instead of hardcoding it.
bootstrap/bootstrap.go: Line 34 (metricsServer := metrics.NewServer(logger, 9091))Analysis chain
Approved: Integration of
metricsServerin the startup process.The creation of
metricsServerand its integration into the application's startup process are well-implemented. However, consider parsing the port number fromprometheus.ymlfor consistency with configuration management practices, as previously discussed.Also applies to: 143-143
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if port numbers are consistent across configuration files. # Test: Search for port number usage in configuration files. rg '9091' -- 'config/'Length of output: 20
Script:
#!/bin/bash # Description: Verify if port numbers are consistent across the entire codebase. # Test: Search for port number usage in the entire codebase. rg '9091'Length of output: 5159
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .gitignore (1 hunks)
- Makefile (1 hunks)
- api/api.go (24 hunks)
- api/debug.go (4 hunks)
- bootstrap/bootstrap.go (5 hunks)
- metrics/collector.go (1 hunks)
- metrics/noop_collector.go (1 hunks)
- metrics/prometheus.yml (1 hunks)
- metrics/server.go (1 hunks)
Additional context used
yamllint
metrics/prometheus.yml
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
golangci-lint
api/api.go
146-146: defers: call to time.Since is not deferred
(govet)
Additional comments not posted (13)
.gitignore (1)
9-9: Approved addition of metrics data directory.The inclusion of
metrics/data/in the.gitignorefile is a good practice to avoid committing generated data to the repository.metrics/noop_collector.go (1)
1-10: NoopCollector implementation is appropriate.The
NoopCollectorserves as a placeholder or default implementation where no metrics collection is required. This implementation is clean and follows the noop pattern correctly.Makefile (1)
47-47: Approved: Cleanup of metrics data directory in thestart-localtarget.The addition of
rm -rf metrics/data/ensures that Prometheus metrics data is cleaned up before each local run, which is a good practice to prevent data persistence issues during development.metrics/server.go (3)
22-35: Approved: Implementation of the Prometheus metrics server setup.The
NewServerfunction correctly sets up an HTTP server dedicated to serving Prometheus metrics. The use ofhttp.NewServeMux()andpromhttp.Handler()is standard for such setups.
43-51: Approved: Robust error handling in the metrics server.The error handling within the goroutine correctly distinguishes between a normal shutdown (
http.ErrServerClosed) and other errors, which is a good practice for clear operational logging.
62-72: Approved: Proper lifecycle management using Go channels.The
Donemethod's implementation using a channel and a context with timeout for server shutdown is a clean and effective way to manage the server lifecycle.api/debug.go (1)
39-39: Clarify the use ofNoopCollectorin error handling.The use of
NoopCollectorin error handling functions across several methods raises questions about whether this is the intended permanent approach or if a more comprehensive metrics collection strategy is planned. Given the previous discussion about potentially usingDefaultCollector, it might be worthwhile to revisit this decision.Also applies to: 51-51, 72-72
Verification successful
Clarify the use of
NoopCollectorin error handling.The use of
NoopCollectorinapi/debug.gofor error handling appears to be intentional to avoid collecting metrics during error handling. TheDefaultCollectoris defined and used within themetricspackage, likely for more comprehensive metrics collection in other contexts. Therefore, the current approach usingNoopCollectorseems appropriate for the intended purpose.
api/debug.go: UsesNoopCollectorfor error handling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if `DefaultCollector` or other collectors are defined and used elsewhere in the project. # Test: Search for other collectors in the metrics package. rg --type go 'Collector' -- 'metrics/'Length of output: 707
Script:
#!/bin/bash # Description: Search for instantiations and usages of NoopCollector and DefaultCollector. # Search for instantiations and usages of NoopCollector. rg --type go 'NoopCollector' -- 'metrics/' # Search for instantiations and usages of DefaultCollector. rg --type go 'DefaultCollector' -- 'metrics/'Length of output: 577
Script:
#!/bin/bash # Description: Search for instantiations and usages of NoopCollector and DefaultCollector in the entire codebase. # Search for instantiations and usages of NoopCollector. rg --type go 'NoopCollector' # Search for instantiations and usages of DefaultCollector. rg --type go 'DefaultCollector'Length of output: 833
bootstrap/bootstrap.go (1)
Line range hint
318-329: Approved: Setup ofmetricsCollectorand its integration in API services.The creation and integration of
metricsCollectorinto the blockchain API setup are correctly implemented, ensuring that metrics are collected throughout the application's operation.api/api.go (5)
9-9: Added import fortime.This import is necessary for the new timing metrics functionality. Ensure it is used appropriately in the code.
23-23: Added import formetrics.This import is crucial for enabling the metrics collection features. It appears to be correctly used in the subsequent changes.
89-89: Addedcollectorfield toBlockChainAPIstruct.This addition is essential for the metrics collection functionality. The field is properly typed as
metrics.Collector, which should facilitate the integration of Prometheus metrics.
Line range hint
101-119: ModifiedNewBlockChainAPIconstructor to includecollector.The addition of the
collectorparameter is necessary for initializing the metrics collection. The function correctly handles the new parameter and integrates it into theBlockChainAPIstruct.
154-154: Refactored error handling to include metrics collection.The
handleErrorfunction has been modified across various methods to include a call tocollector.ApiErrorOccurred(). This is a crucial change for accurate error metrics reporting.Ensure that this pattern is consistently applied across all error handling in the project to maintain uniformity.
Also applies to: 230-230, 235-235, 252-252, 257-257, 275-275, 286-286, 304-304, 314-314, 331-331, 336-336, 341-341, 360-360, 365-365, 393-393, 399-399, 404-404, 434-434, 441-441, 460-460, 483-483, 512-512, 517-517, 571-571, 585-585, 590-590, 609-609, 615-615, 621-621, 689-689, 695-695
|
Are we going to run prometheus back-end as a side car ? or is it gonna be a separate server running independently? |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- api/api.go (24 hunks)
- bootstrap/bootstrap.go (5 hunks)
- go.mod (2 hunks)
- metrics/collector.go (1 hunks)
- metrics/noop_collector.go (1 hunks)
- metrics/prometheus.yml (1 hunks)
Files skipped from review due to trivial changes (2)
- go.mod
- metrics/prometheus.yml
Files skipped from review as they are similar to previous changes (4)
- api/api.go
- bootstrap/bootstrap.go
- metrics/collector.go
- metrics/noop_collector.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- metrics/prometheus.yml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- metrics/prometheus.yml
With `api_request_duration` we can track variety of metrics. For instance: - Measure end-to-end request/response time by method call - API requests per time interval metric - API calls by API endpoint (most used to least used calls)
* Add metrics handler for server * Add test for prometheus config file parsing
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- api/api.go (31 hunks)
- api/debug.go (4 hunks)
- api/server.go (5 hunks)
- bootstrap/bootstrap.go (7 hunks)
- config/config.go (2 hunks)
- metrics/collector.go (1 hunks)
- metrics/config_test.go (1 hunks)
- metrics/handler.go (1 hunks)
- metrics/server.go (1 hunks)
- tests/helpers.go (1 hunks)
- tests/integration_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- metrics/collector.go
Files skipped from review as they are similar to previous changes (2)
- bootstrap/bootstrap.go
- metrics/server.go
Additional comments not posted (28)
metrics/handler.go (1)
17-22: LGTM!The
NewHttpHandlerfunction is correctly implemented.api/debug.go (4)
23-28: LGTM!The
NewDebugAPIfunction is correctly implemented.
Line range hint
36-41:
LGTM!The
TraceTransactionfunction is correctly implemented.
Line range hint
48-53:
LGTM!The
TraceBlockByNumberfunction is correctly implemented.
Line range hint
69-74:
LGTM!The
TraceBlockByHashfunction is correctly implemented.metrics/config_test.go (2)
8-101: LGTM!The
TestReadPortFromConfigFilefunction is correctly implemented.
103-131: LGTM!The
TestExtractPortFromTargetfunction is correctly implemented.tests/integration_test.go (2)
62-75: LGTM! Ensure Prometheus configuration file exists.The addition of the
PrometheusConfigFilePathfield is correct. Ensure that the file./metrics/prometheus.ymlexists and is correctly configured.
199-212: LGTM! Ensure Prometheus configuration file exists.The addition of the
PrometheusConfigFilePathfield is correct. Ensure that the file./metrics/prometheus.ymlexists and is correctly configured.tests/helpers.go (1)
157-158: LGTM! Ensure Prometheus configuration file exists.The addition of the
PrometheusConfigFilePathfield is correct. Ensure that the file./metrics/prometheus.ymlexists and is correctly configured.api/server.go (3)
54-55: LGTM!The addition of the
collectorfield to thehttpServerstructure is correct and enhances the server's functionality by integrating a metrics collector.
Line range hint
64-82:
LGTM!The updates to the
NewHTTPServerfunction are correct and necessary for integrating the metrics collector.
184-185: LGTM!The updates to the
Startfunction are correct and enhance the server's capabilities by providing insights into its performance and usage.config/config.go (2)
96-97: Addition ofPrometheusConfigFilePathfield approved.This addition enhances the configurability of the application by allowing the specification of a Prometheus configuration file path.
160-160: Addition of flag forPrometheusConfigFilePathapproved.This addition allows users to specify the Prometheus configuration file location via command-line flags, enhancing deployment flexibility.
api/api.go (13)
24-24: Addition ofmetricsimport approved.This import is necessary for integrating Prometheus metrics.
90-90: Addition ofcollectorfield toBlockChainAPIstruct approved.This addition enables the collection of Prometheus metrics within the
BlockChainAPImethods.
102-102: Addition ofcollectorparameter toNewBlockChainAPIfunction approved.This addition allows the
BlockChainAPIto be initialized with a metrics collector.
120-120: Modification to includecollectorfield inNewBlockChainAPIfunction approved.This modification ensures that the
collectorfield is properly initialized when creating a newBlockChainAPIinstance.
152-152: Modification to includecollectorfor error reporting inBlockNumbermethod approved.This modification enhances error reporting by utilizing the
collectorto track API errors.
171-177: Modification to includecollectorfor error reporting inSyncingmethod approved.This modification enhances error reporting by utilizing the
collectorto track API errors.
200-200: Modification to includecollectorfor error reporting inSendRawTransactionmethod approved.This modification enhances error reporting by utilizing the
collectorto track API errors.
220-225: Modification to includecollectorfor error reporting inGetBalancemethod approved.This modification enhances error reporting by utilizing the
collectorto track API errors.
242-247: Modification to includecollectorfor error reporting inGetTransactionByHashmethod approved.This modification enhances error reporting by utilizing the
collectorto track API errors.
Line range hint
265-276:
Modification to includecollectorfor error reporting inGetTransactionByBlockHashAndIndexmethod approved.This modification enhances error reporting by utilizing the
collectorto track API errors.
Line range hint
294-304:
Modification to includecollectorfor error reporting inGetTransactionByBlockNumberAndIndexmethod approved.This modification enhances error reporting by utilizing the
collectorto track API errors.
321-331: Modification to includecollectorfor error reporting inGetTransactionReceiptmethod approved.This modification enhances error reporting by utilizing the
collectorto track API errors.
917-919: Modification to includecollectorfor error reporting inhandleErrorfunction approved.This modification ensures that API errors are tracked using the
collector.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- api/api.go (31 hunks)
- api/debug.go (4 hunks)
- api/server.go (5 hunks)
- bootstrap/bootstrap.go (7 hunks)
- go.mod (4 hunks)
Files skipped from review as they are similar to previous changes (5)
- api/api.go
- api/debug.go
- api/server.go
- bootstrap/bootstrap.go
- go.mod
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- api/server.go (5 hunks)
- metrics/handler.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- api/server.go
Additional comments not posted (2)
metrics/handler.go (2)
19-24: LGTM!The
NewHttpHandlerfunction correctly initializes and returns anHttpHandlerinstance.
26-36: Consider logging errors before returning.Logging errors can help in debugging and monitoring.
- if err != nil { - return - } + if err != nil { + h.collector.LogError(err) + return + }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- api/server.go (5 hunks)
- metrics/handler.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- api/server.go
- metrics/handler.go
|
@sideninja Please, take a look at this again once you have time |
|
@illia-malachyn can you update base branch again sorry |
Closes: #359
This covers the following requirements:
Measure end-to-end request/response time by method call (track the time taken from the start of a request to the return of the response for each method call to understand relative performance and user experience using percentiles).
api_request_duration_seconds_bucketapi_request_duration_seconds_bucket{resolver="SendRawTransaction"}API requests per time interval metric
rate(api_request_duration_seconds_count{resolver="SendRawTransaction"}[5m])API calls by API endpoint (most used to least used calls)
sort_desc(sum by(resolver) (rate(api_request_duration_seconds_count{resolver="SendRawTransaction"}[5m])))API errors should be submitted to a counter metric
api_errors_totalSummary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
.gitignoreto adjust directory exclusions.Makefileto clean up themetrics/data/directory before running the application.