Closed collinvandyck closed 4 years ago
The panic I saw was generated from a program that pulls metrics from the jolokia agent running on a kafka connect JVM. It calls the API, deserializes the results (which for the most part are valid), and then iterates through each result and calls stats.Set
. Here's a rough approximation:
func TestPanic(t *testing.T) {
fromAPI := map[string]interface{}{
"valid-value": 0.123,
"invalid-value": map[string]interface{}{
"some": "other",
"data": "from",
"jolokia": "api",
},
}
for k, v := range fromAPI {
stats.Set(k, v)
}
}
Which then generates this panic stacktrace:
panic: stats.ValueOf received a value of unsupported type [recovered]
panic: stats.ValueOf received a value of unsupported type
goroutine 20 [running]:
testing.tRunner.func1(0xc00012a100)
/snap/go/4762/src/testing/testing.go:874 +0x3a3
panic(0x778740, 0x87b410)
/snap/go/4762/src/runtime/panic.go:679 +0x1b2
github.com/segmentio/stats.ValueOf(0x790ce0, 0xc0000b73b0, 0x0, 0xc000072cb0)
/home/collin/code/go/pkg/mod/github.com/segmentio/stats@v4.1.0+incompatible/value.go:50 +0x4e5
github.com/segmentio/stats.MakeField(0x0, 0x0, 0x790ce0, 0xc0000b73b0, 0x1, 0x806e3d, 0xd, 0xc0000c0240, 0x3f)
/home/collin/code/go/pkg/mod/github.com/segmentio/stats@v4.1.0+incompatible/field.go:13 +0x39
github.com/segmentio/stats.(*Engine).measure(0xc0000c6300, 0x806e3d, 0xd, 0x790ce0, 0xc0000b73b0, 0x1, 0x0, 0x0, 0x0)
/home/collin/code/go/pkg/mod/github.com/segmentio/stats@v4.1.0+incompatible/engine.go:122 +0x13e
github.com/segmentio/stats.(*Engine).Set(...)
/home/collin/code/go/pkg/mod/github.com/segmentio/stats@v4.1.0+incompatible/engine.go:94
github.com/segmentio/stats.Set(...)
/home/collin/code/go/pkg/mod/github.com/segmentio/stats@v4.1.0+incompatible/engine.go:236
debezium-service/internal/monitor.TestPanic(0xc00012a100)
/home/collin/code/segment/debezium-service/internal/monitor/metrics_test.go:20 +0x2c9
testing.tRunner(0xc00012a100, 0x81e738)
/snap/go/4762/src/testing/testing.go:909 +0xc9
created by testing.(*T).Run
/snap/go/4762/src/testing/testing.go:960 +0x350
My original thought is that I could do something like this instead:
for k, v := range fromAPI {
if stats.IsValidValue(v) { stats.Set(k,v) }
}
What do you think about supporting an API like this?
for k, v := range fromAPI {
if x := stats.ValueOf(v); x.Type() != stats.Invalid {
stats.Set(k, x)
}
}
That would be totally fine. If we're going to do that though we'd need to push the panic logic to all of the callers of ValueOf, or perhaps create a MustValueOf
func:
func MustValueOf(v Value) Value {
if v.Type() == stats.Invalid {
panic("stats.ValueOf received a value of unsupported type")
}
return v
}
And then change the two production call sites and the myriad of test sites to use MustValueOf(ValueOf(v))
Thoughts?
Sounds good to me 👍
I don't think it's necessary to change the tests since they're not validating the panic case.
We'll probably need to add a case to ValueOf
to handle the condition where it's passed a Value
as well, something like this:
func ValueOf(v interface{}) Value {
switch x := v.(type) {
...
case Value:
return x
default:
return Value{typ: Invalid}
}
}
ready for review
For programs which import opaque metrics from 3rd party APIs and then pass those into the stats package, this will help them avoid panics that result from passing in invalid values (such as structs or maps) that the 3rd party API might have returned.