Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prometheus Metrics for Mux Router #321

Merged
merged 5 commits into from
Nov 16, 2021

Conversation

onelapahead
Copy link
Contributor

@onelapahead onelapahead commented Nov 13, 2021

TODOs

Description

Closes #189

Provides useful histograms for measuring API endpoint latency. for example:

 curl http://localhost:5000/metrics
...
# HELP apiserver_rest_request_duration_seconds Histogram of the request duration
# TYPE apiserver_rest_request_duration_seconds histogram
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="0.005"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="0.01"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="0.025"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="0.05"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="0.1"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="0.25"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="0.5"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="1"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="2.5"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="5"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="10"} 0
apiserver_rest_request_duration_seconds_bucket{code="200",host="127.0.0.1:5000",method="POST",route="/api/v1/network/nodes/self",le="+Inf"} 1

Really just a prototype PR to get simple metrics working and exposed. Next PR would be to actually try to instrument something like batchPin or a plugin and see what refactoring would need to be done around how the Prometheus registry
is handled and provided.

Should there be a test in server_test.go that actually asserts the /metrics route is present or returns a response?

Signed-off-by: hfuss <haydenfuss@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2021

Codecov Report

Merging #321 (cd42330) into main (feac44a) will not change coverage.
The diff coverage is 100.00%.

❗ Current head cd42330 differs from pull request most recent head ec88ca4. Consider uploading reports for the commit ec88ca4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main      #321   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          231       231           
  Lines        12542     12560   +18     
=========================================
+ Hits         12542     12560   +18     
Impacted Files Coverage Δ
internal/apiserver/server.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update feac44a...ec88ca4. Read the comment docs.

@onelapahead onelapahead marked this pull request as draft November 13, 2021 14:08
Signed-off-by: hfuss <haydenfuss@gmail.com>
@onelapahead onelapahead marked this pull request as ready for review November 15, 2021 14:48
Signed-off-by: hfuss <haydenfuss@gmail.com>
Signed-off-by: hfuss <haydenfuss@gmail.com>
Signed-off-by: hfuss <haydenfuss@gmail.com>
Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @hfuss 👍

Feels like the next step would be the first internal metric. I don't know if you have any pointers to @shorsher or others who might want to add in metrics, for how to use the framework you've added.

I see there's metrics.Registry() to get the Prometheus registry object. Should code simply use that directly? Or should there be some internal wrapper interface to do things like increment/add statistics/counters?

@onelapahead
Copy link
Contributor Author

@peterbroadhurst yep would love to do / help with the first internal metric for something like batch or batchPin in the next PR. @shorsher lemme know if you wanna sit down and work on this for an hour at some point soon.

I see there's metrics.Registry() to get the Prometheus registry object. Should code simply use that directly? Or should there be some internal wrapper interface to do things like increment/add statistics/counters?

I think we'd only want to create our own wrappers for metrics primitives if we want to support exporting metrics to other formats / publishers like StatsD. Otherwise, in the next PR I'm thinking we'll add functions like metrics.WithCounter (similar to logging) to the internal/metrics package and that will ensure the Prometheus counter returned is registered properly and so primitives can be fetched via their name.

But figured when we go to add the first few internal metrics that will best inform us how to organize / refactor the metrics package.

@peterbroadhurst
Copy link
Contributor

Fantastic - thanks @hfuss.
Please feel free to merge this PR then, and follow-ups can happen on future PRs.

@peterbroadhurst peterbroadhurst merged commit 7e4fa6a into hyperledger:main Nov 16, 2021
@peterbroadhurst peterbroadhurst deleted the prometheus-poc branch November 16, 2021 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prometheus Instrumentation
3 participants