-
Notifications
You must be signed in to change notification settings - Fork 12
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
Metrics update names + structure (merge pending) #1612
Conversation
…nmarshal, some rename of fields (with Backwards Compatibility to Prometheus), important names of metrics are const to avoid name change issues, all access in e2e and gamma is nicer.
… for tests/e2e/docker to read nested fields Merge branch 'master' of https://github.com/orbs-network/orbs-network-go into feature/metrics-json
@@ -10,20 +10,27 @@ import ( | |||
type StatusResponse struct { | |||
Timestamp time.Time | |||
Status string | |||
Error string | |||
Error string `json:",omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only a couple of minor comments please see below. great PR
instrumentation/metric/rate_test.go
Outdated
@@ -14,7 +14,7 @@ import ( | |||
|
|||
func testRateMeasure(t *testing.T, measure func(rate *Rate)) { | |||
start := time.Now() | |||
rate := newRateWihStart("tps", start) | |||
rate := newRateWihStart("tps", "tps", start) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with?
instrumentation/metric/reader.go
Outdated
nameLen := len(nameParts) | ||
|
||
knownType := isNameAKnownType(nameParts[nameLen-1]) | ||
if nameLen > 0 && knownType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nameLen
is always positive otherwise you would panic on the previous line no? maybe you want to check that it's greater than 1 here to ensure the resulting name will not be empty?
func (t *Text) Export() interface{} { | ||
value := t.value.Load().(string) | ||
var x []interface{} | ||
if err := json.Unmarshal([]byte(value), &x); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment to explain why unmarshaling is required if this is a text type? is it to parse escape characters?
require.True(t, blockHeight > int64(CannedBlocksFileMinHeight)) | ||
} | ||
|
||
func localWrite(t *testing.T, data []byte, filename string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you check that you can write the output to a file in this test?
if this remains, please notice the error message below isn't w
Some name changes (with Backwards compatible for Prometheus)
each metric now has no "name"/"Value" pair ... only value.
a few "string" value metrics are also unmarshaled (topology, committee)
Structure of /metrics is now more flat
Structure of /status is nested json
fix /status to show error string when needed