telemetry: report heartbeat metrics as JSON numbers #3802
Conversation
winder
left a comment
winder
left a comment
There was a problem hiding this comment.
Seems fine to me. You force telemetry to start locally with -t 1, that would create the index for devnet so you can verify that the mapping is correct.
| Channel string `json:"channel"` | ||
| Branch string `json:"branch"` | ||
| CommitHash string `json:"commit-hash"` | ||
| } `json:"Metrics"` // backwards compatible name |
Codecov Report
@@ Coverage Diff @@
## master #3802 +/- ##
==========================================
+ Coverage 49.83% 49.86% +0.03%
==========================================
Files 392 392
Lines 68719 68707 -12
==========================================
+ Hits 34243 34260 +17
+ Misses 30716 30691 -25
+ Partials 3760 3756 -4
Continue to review full report at Codecov.
|
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
Looks good, one question
| a.Equal(hb, data[0]["details"]) | ||
|
|
||
| // assert JSON serialization is backwards compatible | ||
| js, err := json.Marshal(data[0]) |
There was a problem hiding this comment.
maybe just store an old serialized value and compare against new?
There was a problem hiding this comment.
I couldn't figure out how to write a test that had access to the serialized value that the telemetry system makes ... there is a JSON conversion going on inside the logrus hook in a third party library, before being sent via the telemetry client. So I figured I would just simulate that call to json.Marshal here..
brianolson
left a comment
brianolson
left a comment
There was a problem hiding this comment.
Lots of good. I think one more bit of comment+test will make it more durable.
|
@winder @brianolson @algorandskiy addressed CR feedback and ready for re-review |
5 participants
Summary
The current "Heartbeat" telemetry event uses JSON strings inside a
Metricsobject to encode counter values. This PR sends them as JSON numbers instead in a newmobject. For backwards compatibility the StringGaugeversion,version-num,channel,branch, andcommit-hashstring metadata included in the oldMetricsobject are preserved.This PR also fixes a bug where counters with multiple labels were not being reported correctly (only one of the labeled values was being reported, with each iteration over the labels overwriting the reported value for the the base metric name). Label names and values are now added to the reported metric names, sanitized with
_for non-alphanumeric unsupported characters.Test Plan
Slightly updated old tests. Added a new test to assert the new message JSON serialization created by calling
logTelemetrywith this event type.