telemetry: report heartbeat metrics as JSON numbers #3802
Conversation
| 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.
|
| 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..
|
@winder @brianolson @algorandskiy addressed CR feedback and ready for re-review |
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.