[Access] Fixed REST API metrics #4288

Guitarheroua · 2023-04-27T18:55:51Z

Context

This PR addresses the issues regarding the metrics on the Access node's REST interface not being scraped correctly by Prometheus, which caused a memory leak.

The metrics have been fixed and now show up in the Prometheus as expected. However, the memory leak described in the original task was not reproducible. Therefore, this PR solely focuses on fixing the metrics issue.

peterargue · 2023-04-28T17:37:59Z

any idea if this will address the memory leak: #3968

codecov-commenter · 2023-04-28T17:50:13Z

Codecov Report

Merging #4288 (83b4abf) into master (68f607a) will increase coverage by 1.66%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master    #4288      +/-   ##
==========================================
+ Coverage   53.74%   55.41%   +1.66%     
==========================================
  Files         871      783      -88     
  Lines       80761    70019   -10742     
==========================================
- Hits        43408    38801    -4607     
+ Misses      33921    28323    -5598     
+ Partials     3432     2895     -537

Flag	Coverage Δ
unittests	`55.41% <20.00%> (+1.66%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
engine/access/rest/middleware/metrics.go	`0.00% <0.00%> (ø)`
engine/access/rest/server.go	`0.00% <0.00%> (ø)`
module/metrics/access.go	`0.00% <0.00%> (ø)`
module/metrics/rest_api.go	`0.00% <0.00%> (ø)`
engine/access/rpc/engine.go	`59.07% <50.00%> (+0.15%)`	⬆️
engine/access/rest/router.go	`100.00% <100.00%> (ø)`
engine/access/rest/test_helpers.go	`87.50% <100.00%> (+0.54%)`	⬆️

... and 99 files with indirect coverage changes

Guitarheroua · 2023-04-28T18:45:43Z

any idea if this will address the memory leak: #3968

I have tested making 500k REST requests using multiple clients over prolonged periods of time in different runs and prof hasn't reported memory leaks both in used or claimed objects even though metrics were reported correctly in Prometheus(checked using Grafana)

peterargue · 2023-05-01T18:13:44Z

engine/access/rest/middleware/metrics.go

@@ -13,14 +13,17 @@ import (
 	"github.com/gorilla/mux"
 )

+// we have to use single rest collector for all metrics since it's not allowed to register same
+// collector multiple times.
+var restCollector = metrics.NewRestCollector(metricsProm.Config{Prefix: "access_rest_api"})


there should only ever be a single REST server running on a node. Is MetricsMiddleware getting called multiple times in production?

Rather than using a global, how about we build the collector externally and pass it into MetricsMiddleware(), or pass in the prefix value?

You are correct that in production there only be a single REST server running on a node, but the fix we implemented was primarily for tests. During each test, we allocate a new router without a server, which creates a new middleware with a REST controller. However, I discovered that the controller instances were not being unregistered when the test finished, resulting in multiple instances and the error message "duplicate metrics collector registration attempted." Using restCollector as a global variable was the simplest solution to this issue without making significant code changes.

I understand it will be a bit more refactoring, but I think we should align this with how metrics are used elsewhere and pass the collector in rather than create a special case.

what this may look like is creating the collector object in the AN's node builder, and passing it through into the middleware, similar to how it works with the logger. In tests that are not explicitly testing metrics, we can use the NoopCollector which is already setup with stubs for the RestCollector:

flow-go/module/metrics/noop.go

Lines 242 to 247 in 16c25ea

func (nc *NoopCollector) ObserveHTTPRequestDuration(context.Context, httpmetrics.HTTPReqProperties, time.Duration) {

}

func (nc *NoopCollector) ObserveHTTPResponseSize(context.Context, httpmetrics.HTTPReqProperties, int64) {

}

func (nc *NoopCollector) AddInflightRequests(context.Context, httpmetrics.HTTPProperties, int) {}

func (nc *NoopCollector) AddTotalRequests(context.Context, string, string) {}

A pattern we use in the network layer, is to embed sub-collectors in the top level struct and interface:

flow-go/module/metrics/network.go

Lines 23 to 29 in 16c25ea

type NetworkCollector struct {

*UnicastManagerMetrics

*LibP2PResourceManagerMetrics

*GossipSubMetrics

*GossipSubScoreMetrics

*GossipSubLocalMeshMetrics

*AlspMetrics

that way the single top level collector can be passed through.

The RestCollector could be embedded in the AccessCollector interface and implementation in a similar way.

bluesign · 2023-05-03T19:47:17Z

ah I was just gonna debug this on emulator side, metrics were not collected right?

Guitarheroua · 2023-05-04T08:23:59Z

Ah I was just gonna debug this on the emulator side, metrics were not collected, right?

You are correct. The fix was added because metrics were not being collected.

peterargue · 2023-05-15T17:00:24Z

module/metrics/rest_api.go

-}
-
-type recorder struct {
+type RecorderCollector struct {


Let's call this RestCollector. the previous "recorder" terminology is a bit confusing

peterargue

Added one naming comment, but otherwise this looks great. Thanks @Guitarheroua!

Guitarheroua · 2023-05-15T21:28:00Z

Added one naming comment, but otherwise, this looks great. Thanks @Guitarheroua!

I think. this one also should be reviewed onflow/flow-emulator#375, as they depends on each other

zhangchiqing · 2023-06-08T03:38:34Z

engine/access/rest/middleware/metrics.go

 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 			// This is a custom metric being called on every http request
-			r.AddTotalRequests(req.Context(), req.Method, req.URL.Path)
+			restCollector.AddTotalRequests(req.Context(), req.Method, req.URL.Path)


This seems to create a unique value of handler for every single collection, causing too. much metrics to be collected

franklywatson · 2023-06-08T15:37:31Z

@Guitarheroua is taking a look, thanks for catching this

4452: [Metrics] Fix issue with collecting REST Metrics r=peterargue a=Guitarheroua This fix is implemented in response to [a comment made in PR 4288 ](#4288 (comment)). After enabling metrics for REST, the system now generates metrics for each HTTP request based on the request method and URL path. However, when a path includes an ID, such as '**/v1/collection/12345/**' or '**/v1/collection/67890/**', a unique value is created for each path, leading to an issue. To address this problem, I have modified the reporting logic to generate metrics based on the service ID and request method. Currently, the service ID is empty, but there is potential for improvement in future iterations. Co-authored-by: Andrii Slisarchuk <andriyslisarchuk@gmail.com> Co-authored-by: Andrii Slisarchuk <Guitarheroua@users.noreply.github.com> Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>

Fixed metrics for Access node Rest interface.

e32223c

Guitarheroua changed the title ~~[Access] Fixed metrics for Access node Rest interface.~~ [Access] Fixed REST API metrics Apr 27, 2023

Guitarheroua marked this pull request as ready for review April 27, 2023 19:09

Guitarheroua and others added 4 commits April 28, 2023 22:37

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

72e18ba

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

921c246

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

242a02e

Fixed test failed issue with multiple metrics collectors

8e96cc3

Guitarheroua requested a review from peterargue as a code owner May 1, 2023 16:48

peterargue reviewed May 1, 2023

View reviewed changes

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

8f55621

sideninja approved these changes May 3, 2023

View reviewed changes

bluesign mentioned this pull request May 4, 2023

Provide Requests Output For Scripts and Transactions onflow/flow-emulator#226

Closed

Guitarheroua and others added 5 commits May 8, 2023 11:05

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

b79c693

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

f9c6520

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

63e922d

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

83d0d1e

Moved RestMetrics to Access Metrics

bf45732

Guitarheroua mentioned this pull request May 15, 2023

[Access] Fixed REST API metrics onflow/flow-emulator#375

Merged

peterargue reviewed May 15, 2023

View reviewed changes

peterargue approved these changes May 15, 2023

View reviewed changes

Guitarheroua and others added 2 commits May 16, 2023 00:18

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

72c34c4

Fixed review remarks

a9de6c1

Merge branch 'master' into guitarheroua/3971-fix-rest-api-metrics

83b4abf

peterargue merged commit b5eee78 into onflow:master May 16, 2023

zhangchiqing reviewed Jun 8, 2023

View reviewed changes

Guitarheroua mentioned this pull request Jun 8, 2023

[Metrics] Fix issue with collecting REST Metrics #4452

Merged

Guitarheroua deleted the guitarheroua/3971-fix-rest-api-metrics branch June 12, 2023 18:27

peterargue mentioned this pull request Jul 18, 2023

[Access] Fix issues with REST Metrics - v0.31 backport #4569

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Access] Fixed REST API metrics #4288

[Access] Fixed REST API metrics #4288

Guitarheroua commented Apr 27, 2023

peterargue commented Apr 28, 2023 •

edited

Loading

codecov-commenter commented Apr 28, 2023 •

edited

Loading

Guitarheroua commented Apr 28, 2023

peterargue May 1, 2023

Guitarheroua May 4, 2023

peterargue May 9, 2023

bluesign commented May 3, 2023

Guitarheroua commented May 4, 2023

peterargue May 15, 2023

peterargue left a comment

Guitarheroua commented May 15, 2023 •

edited

Loading

zhangchiqing Jun 8, 2023

franklywatson commented Jun 8, 2023

	func (nc *NoopCollector) ObserveHTTPRequestDuration(context.Context, httpmetrics.HTTPReqProperties, time.Duration) {
	}
	func (nc *NoopCollector) ObserveHTTPResponseSize(context.Context, httpmetrics.HTTPReqProperties, int64) {
	}
	func (nc *NoopCollector) AddInflightRequests(context.Context, httpmetrics.HTTPProperties, int) {}
	func (nc *NoopCollector) AddTotalRequests(context.Context, string, string) {}

	type NetworkCollector struct {
	*UnicastManagerMetrics
	*LibP2PResourceManagerMetrics
	*GossipSubMetrics
	*GossipSubScoreMetrics
	*GossipSubLocalMeshMetrics
	*AlspMetrics

[Access] Fixed REST API metrics #4288

[Access] Fixed REST API metrics #4288

Conversation

Guitarheroua commented Apr 27, 2023

Context

peterargue commented Apr 28, 2023 • edited Loading

codecov-commenter commented Apr 28, 2023 • edited Loading

Codecov Report

Guitarheroua commented Apr 28, 2023

peterargue May 1, 2023

Choose a reason for hiding this comment

Guitarheroua May 4, 2023

Choose a reason for hiding this comment

peterargue May 9, 2023

Choose a reason for hiding this comment

bluesign commented May 3, 2023

Guitarheroua commented May 4, 2023

peterargue May 15, 2023

Choose a reason for hiding this comment

peterargue left a comment

Choose a reason for hiding this comment

Guitarheroua commented May 15, 2023 • edited Loading

zhangchiqing Jun 8, 2023

Choose a reason for hiding this comment

franklywatson commented Jun 8, 2023

peterargue commented Apr 28, 2023 •

edited

Loading

codecov-commenter commented Apr 28, 2023 •

edited

Loading

Guitarheroua commented May 15, 2023 •

edited

Loading