cli to push metrics to aggregator service #8835
Conversation
kasey
requested review from
farazdagi,
terencechain and
rkapka
and removed request for
a team
shared/clientstats/scrapers.go
Outdated
| var m *dto.Metric | ||
| var ok bool | ||
|
|
||
| f, ok = pf["process_cpu_seconds_total"] |
There was a problem hiding this comment.
If these hard-coded strings change in the future (although unlikely), it would be easier if they lived as consts at the top of the file perhaps
There was a problem hiding this comment.
IMO in an instance like this where the value is used in one place it is more readable to keep the strings inline, but don't feel super strongly about it.
shared/clientstats/scrapers.go
Outdated
| var m *dto.Metric | ||
| var ok bool | ||
|
|
||
| f, ok = pf["beacon_head_slot"] |
There was a problem hiding this comment.
+1 on the hard-coded strings
shared/clientstats/scrapers.go
Outdated
| return bytes.NewBuffer(b), err | ||
| } | ||
|
|
||
| func NewBeaconNodeScraper(promExpoURL string) Scraper { |
There was a problem hiding this comment.
Not too much preference here, but the file would be more readable if we kept structs at the top, constructor functions next, exported functions next, then finally unexported funcs
kasey
force-pushed
the
clientstats-prometheus-changes
branch
from
a5210cb
to
954c185
Compare
kasey
changed the title
kasey
changed the title
kasey
force-pushed
the
clientstats-cli
branch
2 times, most recently
from
b55ae68
to
fae8ea9
Compare
kasey
force-pushed
the
clientstats-prometheus-changes
branch
from
793764a
to
4f7d958
Compare
kasey
force-pushed
the
clientstats-prometheus-changes
branch
from
4f7d958
to
6ea2ab6
Compare
kasey
force-pushed
the
clientstats-cli
branch
2 times, most recently
from
2b6d35d
to
f69f21f
Compare
kasey
changed the title
kasey
force-pushed
the
clientstats-prometheus-changes
branch
from
6ea2ab6
to
99b9e4e
Compare
kasey
force-pushed
the
clientstats-cli
branch
3 times, most recently
from
a6127d7
to
46dc9b6
Compare
kasey
force-pushed
the
clientstats-prometheus-changes
branch
from
4dc9dab
to
a278440
Compare
kasey
force-pushed
the
clientstats-prometheus-changes
branch
from
a278440
to
2e8b491
Compare
Codecov Report
@@ Coverage Diff @@
## develop #8835 +/- ##
===========================================
- Coverage 60.59% 60.55% -0.05%
===========================================
Files 525 527 +2
Lines 36978 37108 +130
===========================================
+ Hits 22406 22469 +63
- Misses 11350 11402 +52
- Partials 3222 3237 +15 |
kasey
force-pushed
the
clientstats-prometheus-changes
branch
from
2e8b491
to
f1dc18e
Compare
shared/clientstats/scrapers_test.go
Outdated
| bnScraper := beaconNodeScraper{} | ||
| bnScraper.tripper = &mockRT{body: prometheusTestBody} | ||
| r, err := bnScraper.Scrape() | ||
| if err != nil { |
There was a problem hiding this comment.
Can be more consistent with rest of the codebase by doing
require.NoError(t, err, Unexpected error calling beaconNodeScraper.Scrape)
There was a problem hiding this comment.
Updated, although I just realized I've been using assert.NoError in multiple places rather than require.NoError. I see the difference is that require uses FatalF and assert uses Errorf. I'll put more care into picking between assert/require going forward.
| // It is used by the cli to write to stdout when an http endpoint | ||
| // is not provided. The output could be piped into another program | ||
| // or used for debugging. | ||
| func NewGenericClientStatsUpdater(w io.Writer) Updater { |
There was a problem hiding this comment.
These are not tested but I think that's perfectly fine
* adds client-stats types beacause they are used by some of the prometheus collection code. * new prometheus collector for db disk size * new prometheus collector for web3 client connection status
What type of PR is this?
Feature
What does this PR do? Why is it needed?
This PR implements a cli to collect the majority of the beaconcha.in beacon-node and validator metrics collection specification. It does not implement the system metrics collector at this time. The purpose of this feature is to allow users who wish to use beaconcha.in monitoring to scrape data from their prysm processes and ship them to beaconcha.in's collection API. The cli gives users the option of specifying any endpoint, so if desired the same tool can be used to post data to a custom API.
Which issues(s) does this PR fix?
Fixes #8833
Other notes for review
This pull request should be merged into
clientstats-prometheus-changesbefore merging it down! The PR for that branch is at #8834This PR will introduce the client stats scraper itself. These 2 PRs have been broken up because the boilerplate of a new cli command adds to the line count and this PR includes changes to some existing core services.