[Access] Fixed REST API metrics #375
Conversation
Guitarheroua
requested review from
sideninja,
turbolent,
janezpodhostnik and
bluesign
as code owners
|
Is it correct that this PR depends on onflow/flow-go#4288? |
Yes, this is correct. |
oh lets not disable please, waiting for it long time :) #226 |
| debugLogger := zerolog.Logger{} | ||
| if debug { | ||
| debugLogger = zerolog.New(os.Stdout) | ||
| } | ||
|
|
||
| srv, err := rest.NewServer(backend.NewAdapter(be), fmt.Sprintf("%s:3333", host), debugLogger, chain) | ||
| restCollector := metrics.NewNoopCollector() |
There was a problem hiding this comment.
sounds like @bluesign would these turned on by default
| restCollector := metrics.NewNoopCollector() | |
| restCollector := metrics.NewRestCollector(metricsProm.Config{Prefix: "access_rest_api"}) |
also needs to import metricsProm
There was a problem hiding this comment.
not necessarily by default, but support an option to enable it
There was a problem hiding this comment.
that's fair, but fwiw it is enabled by default for grpc
Line 74 in 3c8ca15
sounds like we need a followup issue to make metrics configurable for both
There was a problem hiding this comment.
We can separate this in another PR yeah.
…ix-rest-api-metrics
|
@Guitarheroua looks like |
Codecov Report
@@ Coverage Diff @@
## master #375 +/- ##
==========================================
+ Coverage 46.30% 46.31% +0.01%
==========================================
Files 27 27
Lines 3555 3556 +1
==========================================
+ Hits 1646 1647 +1
Misses 1766 1766
Partials 143 143
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@turbolent could you take one final look? I'm not a contributor to this repo and there was a change required for the flow-go version update. |
There was a problem hiding this comment.
@peterargue I don't really know anything about the changes here (FVM state vs snapshot, REST API / metrics), so leave review to who is knowledgeable
onflow/flow-go#3971
Added modifications due to changes in rest server creation. Now it has one more argument RestMetrics.