metrics, cmd/geth: informational metrics (prometheus, influxdb, opentsb) #24877
Conversation
jorgeacortes
requested review from
karalabe,
holiman and
rjl493456442
as code owners
jorgeacortes
force-pushed
the
informational_metrics
branch
from
dabe86c
to
cff0d59
Compare
|
The PR looks good to me. It works as expected and all the related tests passed Screenshots for the added prometheus info
|
metrics/gauge_info.go
Outdated
| lastIdx := len(g) - 1 | ||
| v := "{" | ||
| for idx, entry := range g { | ||
| v += "\"" + entry.Key + "\":\"" + entry.Val + "\"" | ||
| if idx != lastIdx { | ||
| v += "," | ||
| } | ||
| } | ||
| v += "}" |
There was a problem hiding this comment.
This looks fragile, something like this would be more robust:
entries := make([]string, 0, len(g))
for k, v := range g {
entries = append(entries, fmt.Sprintf("%s=%q", k, v))
}
return fmt.Sprintf("{%s}",strings.Join(entries,","))
There was a problem hiding this comment.
This method shouldn't be needed, just use json.Marshal(..) and pass the entries as a key/value map?
In fact, wouldn't it be a better model to have the g be a key/value map instead of a list of entries with key and a value ?
So instead of
type GaugeInfoValue []GaugeInfoEntry
You would have
type GaugeInfoValue map[string]stringThen just do json.Marshal(g) and that's it
metrics/prometheus/collector.go
Outdated
| c.buff.WriteString(fmt.Sprintf("%s {", name)) | ||
| for idx, entry := range value { | ||
| c.buff.WriteString(fmt.Sprintf("%s=\"%s\"", entry.Key, entry.Val)) | ||
| if idx != len(value)-1 { | ||
| c.buff.WriteString(", ") | ||
| } | ||
| } | ||
| c.buff.WriteString("} 1 \n\n") |
There was a problem hiding this comment.
Fragile again:
c.buff.WriteString(fmt.Sprintf("%s {", name))
entries := make([]string, 0, len(value))
for _, entry := range value {
entries = append(entries, fmt.Sprintf("%s=%q", entry.Key, entry.Val))
}
c.buff.WriteString(fmt.Sprintf("{%s}",strings.Join(entries,",")))
c.buff.WriteString("} 1 \n\n")
There was a problem hiding this comment.
Let's just avoid writing json manually, we use the json package for that
metrics/gauge_info.go
Outdated
| lastIdx := len(g) - 1 | ||
| v := "{" | ||
| for idx, entry := range g { | ||
| v += "\"" + entry.Key + "\":\"" + entry.Val + "\"" | ||
| if idx != lastIdx { | ||
| v += "," | ||
| } | ||
| } | ||
| v += "}" |
There was a problem hiding this comment.
This method shouldn't be needed, just use json.Marshal(..) and pass the entries as a key/value map?
In fact, wouldn't it be a better model to have the g be a key/value map instead of a list of entries with key and a value ?
So instead of
type GaugeInfoValue []GaugeInfoEntry
You would have
type GaugeInfoValue map[string]stringThen just do json.Marshal(g) and that's it
…onal metrics - Created GaugeInfo metrics type for registering informational metrics. - Updated all related metrics modules to support the new GaugeInfo type. - Registered chain/info GaugeInfo with the chain_id value. - Registered geth/info GaugeInfo with system and build parameters. Implements ethereum#21783
holiman
force-pushed
the
informational_metrics
branch
2 times, most recently
from
8eb3c7a
to
00367d0
Compare
holiman
changed the title
holiman
changed the title
holiman
force-pushed
the
informational_metrics
branch
2 times, most recently
from
9373869
to
e2a9b98
Compare
|
I have now refactored this quite a bit, and it should we ok for influx, prometheus and opentsb. It is defined for several other endpoints, but most of those seem deprecated and they do not even have the most basic tests. For influx/prometheus, I have now added proper output-tests, to ensure correctnes now and in the future. What I have so far not done yet, however, is to actually hook them up and watch the numbers on actual stats-servers. Or, rather, I've tried to, but my grafana skills are a bit lacking.... |
|
Ok, proof of concept:ish for influxdb/grafana ✔️ :
|
holiman
force-pushed
the
informational_metrics
branch
from
e2a9b98
to
8e61797
Compare
This change moves the initialization of an ordered registry to a shared subpackage in 'internal', and makes it so both influx and prometheus use the same testdata.
holiman
force-pushed
the
informational_metrics
branch
from
7d218ea
to
5627ca1
Compare
…sb) (ethereum#24877) This chang creates a GaugeInfo metrics type for registering informational (textual) metrics, e.g. geth version number. It also improves the testing for backend-exporters, and uses a shared subpackage in 'internal' to provide sample datasets and ordered registry. Implements ethereum#21783 --------- Co-authored-by: Martin Holst Swende <martin@swende.se>
…b, opentsb) (ethereum#24877)" This reverts commit c10e937.
…b, opentsb) (ethereum#24877)" This reverts commit c10e937.
…b, opentsb) (ethereum#24877)" This reverts commit c10e937.
Implements #21783