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

feat: add prometheus support #474

Merged
merged 13 commits into from
Dec 16, 2022
Merged

feat: add prometheus support #474

merged 13 commits into from
Dec 16, 2022

Conversation

entropidelic
Copy link
Contributor

@entropidelic entropidelic commented Dec 15, 2022

Linked to #94

This PR adds support for Prometheus metrics

  • Add --metrics flag to CLI to install a prometheus metrics exporter.
  • Add reth prefix layer
  • Add p2pstream.disconnected_errors metric.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

oh wow, tysm for taking this on!

ref #94

before we rush anything here, I'd like to discuss what:

  • the best crate for metrics for our use case is
  • do we miss out on anything if we fully commit to prometheus, as I understood it the metrics crate is not limited to prometheus, right?

@onbjerg wdyt?

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2022

Codecov Report

Merging #474 (37e2b9b) into main (2b0f531) will decrease coverage by 0.57%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #474      +/-   ##
==========================================
- Coverage   73.67%   73.09%   -0.58%     
==========================================
  Files         240      241       +1     
  Lines       23523    24474     +951     
==========================================
+ Hits        17330    17890     +560     
- Misses       6193     6584     +391     
Impacted Files Coverage Δ
bin/reth/src/cli.rs 0.00% <0.00%> (ø)
crates/net/eth-wire/src/p2pstream.rs 82.44% <0.00%> (-2.19%) ⬇️
crates/storage/db/src/abstraction/cursor.rs 86.66% <0.00%> (-13.34%) ⬇️
crates/net/discv4/src/lib.rs 50.70% <0.00%> (-11.31%) ⬇️
crates/net/discv4/src/config.rs 60.62% <0.00%> (-9.38%) ⬇️
crates/net/common/src/ban_list.rs 61.05% <0.00%> (-6.03%) ⬇️
crates/stages/src/pipeline.rs 90.15% <0.00%> (-5.28%) ⬇️
crates/stages/src/test_utils/test_db.rs 66.10% <0.00%> (-5.09%) ⬇️
crates/net/discv4/src/proto.rs 86.29% <0.00%> (-4.41%) ⬇️
crates/net/network/src/discovery.rs 56.00% <0.00%> (-4.00%) ⬇️
... and 39 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@onbjerg
Copy link
Member

onbjerg commented Dec 15, 2022

I think this is ok for now but I would be wary adding a dockerfile (and a docker-compose file) right now since those quickly turn into bikeshedding in my experience (see foundry churn on the dockerfile) and I would rather not think too much about release stuff at the moment. We should think about not only supporting prometheus at some point but this is an ok starting point, although it will probably be moved around a bit as the CLI evolves :)

So TL;DR think it's ok to merge the metric stuff but would prefer no dockerfile atm (defer to @mattsse on the metric added in the networking crate - maybe it fits better somewhere else)

@onbjerg onbjerg added C-enhancement New feature or request A-observability Related to tracing, metrics, logs and other observability tools labels Dec 15, 2022
@unbalancedparentheses
Copy link

Completely agree with @onbjerg comment. I was telling the same the @entropidelic. We will focus more on the metrics and we will remove the dockerfile

@entropidelic entropidelic marked this pull request as ready for review December 16, 2022 18:15
@entropidelic
Copy link
Contributor Author

thanks @mattsse and @onbjerg for the feedback! The docker related files have been removed from the PR and it is ready for review now

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm, thanks! it will probably experience a bit of churn when config lands

@onbjerg onbjerg merged commit 4da574d into paradigmxyz:main Dec 16, 2022
@laibe
Copy link

laibe commented Dec 21, 2022

having a Dockerfile would be great to have once reth is closer to stable release. Many el and cl clients include one (out of the top of my head geth, nethermind, teku, lighthouse)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-observability Related to tracing, metrics, logs and other observability tools C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants