[Merged by Bors] - api: Add go-http-metrics to collect API metrics #6099
Conversation
kacpersaw
requested review from
dshulyak,
fasmat,
poszu,
ivan4th and
acud
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6099 +/- ##
=======================================
Coverage 81.5% 81.6%
=======================================
Files 302 302
Lines 32478 32488 +10
=======================================
+ Hits 26502 26517 +15
+ Misses 4240 4238 -2
+ Partials 1736 1733 -3 ☔ View full report in Codecov by Sentry. |
api/grpcserver/http_server.go
Outdated
| ) | ||
|
|
||
| // recorder is a global recorder for http metrics. | ||
| var recorder *httpMetrics.Recorder |
There was a problem hiding this comment.
I wonder why is a global variable needed here? StartService is called once in the whole codebase. It can just live as a variable scoped in the method, WDYT?
There was a problem hiding this comment.
Why do we need a dedicated dependency for this? We already use github.com/grpc-ecosystem/go-grpc-middleware which has a metrics provider: https://github.com/grpc-ecosystem/go-grpc-middleware/tree/main/providers/prometheus
Wouldn't this work too?
api/grpcserver/http_server.go
Outdated
| @@ -67,6 +72,7 @@ func (s *JSONHTTPServer) Shutdown(ctx context.Context) error { | |||
| // StartService starts the json api server and listens for status (started, stopped). | |||
| func (s *JSONHTTPServer) StartService( | |||
| ctx context.Context, | |||
| collectMetrics bool, | |||
There was a problem hiding this comment.
In my opinion this flag should be part of the instantiation of the JSONHTTPServer. The NewJSONHTTPServer already takes two values from the app.Config.API object, I see no problem if it takes the whole object.
I think it also makes sense to not use the general app.Config.CollectMetrics flag for this, but instead have a dedicated flag in the API config. Wdyt?
There was a problem hiding this comment.
yep, forgot about this. Fixing.
There was a problem hiding this comment.
@fasmat We can create separate option in API config for this, but then we will need to expose another metrics server.
https://github.com/spacemeshos/go-spacemesh/blob/develop/node/node.go#L2131
If that's ok, let me know
There was a problem hiding this comment.
Why do we have 2 metric servers? Can't the information you collect be exposed at the existing one?
There was a problem hiding this comment.
Yes, that's fine. I was thinking about the case when the user enables metrics in the API but not in the main configuration itself. However, we can validate that.
There was a problem hiding this comment.
ah ok I misunderstood, then yes there should only be one config option (probably the Config.CollectMetrics and that can just be passed to the server 🙂
This middleware does not show metrics for individual endpoints, nor does it indicate whether the response is a success or failure. |
|
bors merge |
|
👎 Rejected by code reviews |
|
bors merge |
## Motivation By integrating go-http-metrics, we can gain detailed insights into the performance and behavior of our APIs.
|
Build failed:
|
|
bors merge |
## Motivation By integrating go-http-metrics, we can gain detailed insights into the performance and behavior of our APIs.
|
Build failed: |
|
bors merge |
## Motivation By integrating go-http-metrics, we can gain detailed insights into the performance and behavior of our APIs.
|
Pull request successfully merged into develop. Build succeeded: |
spacemesh-bors
bot
changed the title
## Motivation By integrating go-http-metrics, we can gain detailed insights into the performance and behavior of our APIs.
[backport] api: Add go-http-metrics to collect API metrics (#6099)
Motivation
By integrating go-http-metrics, we can gain detailed insights into the performance and behavior of our APIs.