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

Add prometheus metrics endpoint #294

Closed
wants to merge 4 commits into from

Conversation

0xste
Copy link

@0xste 0xste commented Sep 5, 2022

πŸ“ Summary

An initial implementation of some prometheus metrics for MEV Boost

β›± Motivation and Context

I want to be able to monitor my mev-boost container with prometheus to get better operational insight

πŸ“š References

#253

βœ… I have run these commands

  • [βœ”οΈ] go mod tidy
  • [βœ”οΈ] make test
  • [βœ”οΈ] make lint
  • [❔] make run-mergemock-integration

Getting an error back from make run-mergemock-integration, but not sure if it's related to my change

ERROR  [2022-09-05T19:09:20+01:00] Unable to retrieve proposal payload           error="builder REST API returned non-200 status code: 204" slot="5"
make: *** [run-mergemock-integration] Error 1```

server/metrics.go Outdated Show resolved Hide resolved
server/service.go Outdated Show resolved Hide resolved
@0xste 0xste changed the title feat: added initial prometheus metrics Add prometheus metrics endpoint Sep 7, 2022
@ybstaked
Copy link

ybstaked commented Oct 5, 2022

what's the progress on this? we also would appreciate having prometheus metrics on boost


| **metric** | **type** | **labels** | **description** |
|--------------------------------|----------|------------|---------------------------------------------|
| mev_boost_validator_info | gauge | pubkey | validator identity information |
Copy link

Choose a reason for hiding this comment

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

wouldn't having a number of validators successfully registered per relay be more helpful? per pubkey might be a lot of data for validators operating at scale

@metachris
Copy link
Collaborator

These changes are deep and extensive into the core system and logic. Considering that mev-boost is run at 50% of all validators, it is too risky and impactful for adding a large amount of new logic in the core flows at the current time.

Perhaps consider collecting the metrics from the logs, or thinking about a possible plugin interface. πŸ€”

@0xste
Copy link
Author

0xste commented Oct 7, 2022

These changes are deep and extensive into the core system and logic. Considering that mev-boost is run at 50% of all validators, it is too risky and impactful for adding a large amount of new logic in the core flows at the current time.

Perhaps consider collecting the metrics from the logs, or thinking about a possible plugin interface. πŸ€”

Thanks for your feedback @metachris, i may pick this up again later and try coming at it from a different angle πŸ‘

@0xste 0xste closed this Oct 7, 2022
@0xste 0xste mentioned this pull request Oct 7, 2022
@ybstaked
Copy link

ybstaked commented Oct 7, 2022

These changes are deep and extensive into the core system and logic. Considering that mev-boost is run at 50% of all validators, it is too risky and impactful for adding a large amount of new logic in the core flows at the current time.

Perhaps consider collecting the metrics from the logs, or thinking about a possible plugin interface. πŸ€”

@metachris as this PR is implemented as is, seems this would've been behind a -metrics flag (similar to prysm and teku have their prometheus metrics behind a flag on a separate metrics port). Making the metrics opt-in and improving the metrics collected, I think this would be a useful feature to add native prometheus metrics

@0xste
Copy link
Author

0xste commented Oct 12, 2022

@ybstaked @metachris happy to reopen and rebase this PR if there's any will to get it implemented

Otherwise the suggestion is the approach outlined in #370

@metachris
Copy link
Collaborator

I sincerely appreciate the effort in adding the prometheus support. As mentioned before, the changes reach too deep into the core program flow to merge and force on a majority of proposers, without extensive review and in particular discussions beforehand. If you feel you'd like to pursue this, then please open an issue and continue the conversation there.

@ybstaked
Copy link

@metachris since there's already an issue open for this here: #253, what are the discussion points?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants