[Metrics] Fix issue with collecting REST Metrics #4452

Guitarheroua · 2023-06-08T19:24:33Z

This fix is implemented in response to a comment made in PR 4288 .

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.

…ection.

zhangchiqing · 2023-06-08T20:20:49Z

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
-			restCollector.AddTotalRequests(req.Context(), req.Method, req.URL.Path)
+			restCollector.AddTotalRequests(req.Context(), serviceID, req.Method)


what would be the method? is it GET/PUT/POST etc?

I think the only value for req.Method is just Get, so since serviceID is empty, it will just be a single metrics for total Get requests?

would it be more useful to have metrics on total requests to different path?
Get /v1/events/*
Get / v1/collections/*

what would be the method? is it GET/PUT/POST etc?

I think the only value for req.Method is just Get, so since serviceID is empty, it will just be a single metrics for total Get requests?

Yes, the method would be GET and POST, as only two types are present in routers.

Would it be more useful to have metrics on total requests to different paths?
Get /v1/events/*
Get / v1/collections/*

Yeah, I did consider that option. However, we have various types of URLs, such as /v1/blocks/{id}/payload or /v1/accounts/{address}, with arguments id and address in different positions. To determine which group each URL belongs to, we would definitely need to implement some pattern-matching mechanism or something similar. Since we can't predict the exact position of this floating argument in a URL, it would require some additional work. The proper fix could be part of the REST refactoring, but for now, we could use this approach as a quick solution.

tbh, if we're only collecting metrics for GET/POST, I don't think it's worth collecting the data.

How about adding something like this:

var routeUrlMap = map[string]string{} var routeRE = regexp.MustCompile("/v1/([\\w]+)(/([\\w]+)(/([\\w]+))?)?") func init() { for _, r := range rest.Routes { routeUrlMap[r.Pattern] = r.Name } } func URLToRoute(url string) (string, error) { normalized, err := normalizeURL(url) if err != nil { return "", err } name, ok := routeUrlMap[normalized] if !ok { return "", fmt.Errorf("invalid url") } return name, nil } func normalizeURL(url string) (string, error) { matches := routeRE.FindAllStringSubmatch(url, -1) if len(matches) != 1 || len(matches[0]) != 6 { return "", fmt.Errorf("invalid url") } // given a URL like // /v1/blocks/1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef/payload // groups [ 1 ] [ 3 ] [ 5 ] // normalized form like /v1/blocks/{id}/payload parts := []string{matches[0][1]} switch len(matches[0][3]) { case 0: // top level resource. e.g. /v1/blocks case 64: // id based resource. e.g. /v1/blocks/1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef parts = append(parts, "{id}") case 16: // address based resource. e.g. /v1/accounts/1234567890abcdef parts = append(parts, "{address}") default: // named resource. e.g. /v1/network/parameters parts = append(parts, matches[0][3]) } if matches[0][5] != "" { parts = append(parts, matches[0][5]) } return "/" + strings.Join(parts, "/"), nil }

then using the return from URLToRoute as the handler name?

It's not very generic but probably good enough in the medium term.

master...petera/add-rest-url-parser

Oh, sorry! I missed messages because my notifications from GitHub go to spam. Yes, it should work, as I see you add a parser.

codecov-commenter · 2023-06-09T08:45:59Z

Codecov Report

Merging #4452 (2f94bd6) into master (12f898b) will decrease coverage by 2.37%.
The diff coverage is 43.86%.

@@            Coverage Diff             @@
##           master    #4452      +/-   ##
==========================================
- Coverage   54.09%   51.73%   -2.37%     
==========================================
  Files         563      726     +163     
  Lines       56220    65570    +9350     
==========================================
+ Hits        30414    33922    +3508     
- Misses      23459    29040    +5581     
- Partials     2347     2608     +261

Flag	Coverage Δ
unittests	`51.73% <43.86%> (-2.37%)`	⬇️

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

Impacted Files	Coverage Δ
admin/commands/storage/read_range_blocks.go	`0.00% <0.00%> (ø)`
...dmin/commands/storage/read_range_cluster_blocks.go	`0.00% <0.00%> (ø)`
cmd/execution_builder.go	`0.00% <0.00%> (ø)`
cmd/execution_config.go	`0.00% <0.00%> (ø)`
cmd/node_builder.go	`93.87% <ø> (-2.07%)`	⬇️
cmd/scaffold.go	`14.66% <0.00%> (+0.16%)`	⬆️
cmd/util/ledger/reporters/export_reporter.go	`0.00% <ø> (ø)`
cmd/utils.go	`20.58% <ø> (+3.51%)`	⬆️
cmd/verification_builder.go	`0.00% <0.00%> (ø)`
config/network/config.go	`0.00% <0.00%> (ø)`
... and 66 more

... and 480 files with indirect coverage changes

…o into guitarheroua/fix-3971-fix-rest-api-metrics

…eroua/fix-3971-fix-rest-api-metrics

peterargue

looks good. Thanks for fixing this up @Guitarheroua !

peterargue · 2023-06-28T21:46:51Z

@Guitarheroua can you take a look at those lint errors?

Guitarheroua · 2023-06-28T23:36:32Z

@Guitarheroua can you take a look at those lint errors?

Hey, could you please take a look again at my last change, there was cyclic dependency and I fix it by passing URLToRoute as an argument to MetricsMiddleware, to avoid new refactoring and moving in this part of code base.

peterargue · 2023-06-29T18:22:48Z

engine/access/rest/middleware/metrics.go

 	metricsMiddleware := middleware.New(middleware.Config{Recorder: restCollector})
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+			routeName, err := urlToRoute(req.URL.Path)
+			if err != nil {
+				return


looks like there are a few REST unittests failing with empty responses. I'm guessing it's related to this return since it effectively drops the request. Can you take a look?

most likely, the tests just need to be updated to use more valid urls/inputs, but not 100% sure.

looks like there are a few REST unit tests failing with empty responses. I'm guessing it's related to this return since it effectively drops the request. Can you take a look?

Most likely, the tests just need to be updated to use more valid urls/inputs, but not 100% sure.

Could I, for example, on error just fill routeName with an "unknown" or "invalid" string? Because this function anyway supposes to return http.Handler

yea, that sounds good. Let's go with unknown

peterargue · 2023-06-29T21:16:57Z

bors merge

bors · 2023-06-29T21:43:28Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

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>

4571: [Access] Update REST metrics to use route name for all types r=peterargue a=peterargue The REST metrics handler passes the original route into the collector, causing high cardinality and performance issues on the prometheus servers. We attempted to fix this with #4452, but only updated 1 of 4 metrics that were reporting the URL. This PR updates all metrics to use the mapped route name. It also removes a bunch of unnecessary config and aligns the collector with the other metrics collectors in flow-go Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>

Guitarheroua and others added 3 commits June 8, 2023 21:54

Fixed isse with rest metrics reports each request path as unique coll…

01d21df

…ection.

Renamed arguments with proper naming.

fd160d4

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

b76dc85

Guitarheroua marked this pull request as ready for review June 8, 2023 20:16

Guitarheroua requested a review from peterargue as a code owner June 8, 2023 20:16

durkmurder requested review from zhangchiqing and peterargue and removed request for peterargue June 8, 2023 20:16

durkmurder assigned zhangchiqing and peterargue Jun 8, 2023

zhangchiqing reviewed Jun 8, 2023

View reviewed changes

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

f2e8a3d

zhangchiqing approved these changes Jun 8, 2023

View reviewed changes

Guitarheroua and others added 6 commits June 9, 2023 14:51

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

87da372

[Access] Add parsing logic for REST urls for metrics

4646bcb

Merge branch 'petera/add-rest-url-parser' of github.com:onflow/flow-g…

f9181f6

…o into guitarheroua/fix-3971-fix-rest-api-metrics

changed rest metrics handler logic.

1b48569

Merge branch 'master' of github.com:Guitarheroua/flow-go into guitarh…

1ec7a2e

…eroua/fix-3971-fix-rest-api-metrics

linted

4b3383a

peterargue approved these changes Jun 26, 2023

View reviewed changes

peterargue requested a review from zhangchiqing June 26, 2023 18:39

Guitarheroua and others added 2 commits June 29, 2023 01:34

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

69a84a7

Fixed cyclic dependency and regexp lint.

a3b4bd5

peterargue reviewed Jun 29, 2023

View reviewed changes

Change error handling in rest metrics.

2f94bd6

peterargue approved these changes Jun 29, 2023

View reviewed changes

bors bot merged commit f5d3aec into onflow:master Jun 29, 2023

peterargue mentioned this pull request Jul 18, 2023

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

Merged

peterargue mentioned this pull request Jul 19, 2023

[Access] Update REST metrics to use route name for all types #4571

Merged

Guitarheroua deleted the guitarheroua/fix-3971-fix-rest-api-metrics branch October 30, 2023 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Metrics] Fix issue with collecting REST Metrics #4452

[Metrics] Fix issue with collecting REST Metrics #4452

Guitarheroua commented Jun 8, 2023

zhangchiqing Jun 8, 2023 •

edited

Loading

zhangchiqing Jun 8, 2023

Guitarheroua Jun 8, 2023

peterargue Jun 12, 2023 •

edited

Loading

peterargue Jun 12, 2023

Guitarheroua Jun 15, 2023

codecov-commenter commented Jun 9, 2023 •

edited

Loading

peterargue left a comment

peterargue commented Jun 28, 2023

Guitarheroua commented Jun 28, 2023

peterargue Jun 29, 2023

Guitarheroua Jun 29, 2023 •

edited

Loading

peterargue Jun 29, 2023

peterargue commented Jun 29, 2023

bors bot commented Jun 29, 2023

[Metrics] Fix issue with collecting REST Metrics #4452

[Metrics] Fix issue with collecting REST Metrics #4452

Conversation

Guitarheroua commented Jun 8, 2023

zhangchiqing Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

zhangchiqing Jun 8, 2023

Choose a reason for hiding this comment

Guitarheroua Jun 8, 2023

Choose a reason for hiding this comment

peterargue Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

peterargue Jun 12, 2023

Choose a reason for hiding this comment

Guitarheroua Jun 15, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jun 9, 2023 • edited Loading

Codecov Report

peterargue left a comment

Choose a reason for hiding this comment

peterargue commented Jun 28, 2023

Guitarheroua commented Jun 28, 2023

peterargue Jun 29, 2023

Choose a reason for hiding this comment

Guitarheroua Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

peterargue Jun 29, 2023

Choose a reason for hiding this comment

peterargue commented Jun 29, 2023

bors bot commented Jun 29, 2023

zhangchiqing Jun 8, 2023 •

edited

Loading

peterargue Jun 12, 2023 •

edited

Loading

codecov-commenter commented Jun 9, 2023 •

edited

Loading

Guitarheroua Jun 29, 2023 •

edited

Loading