Add IsValidValue method to stats package.

[collinvandyck]

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.

[achille-roussel]

Structs and arrays are supported in some places (stats.Report for example).

I wonder if this would be better served by introducing an invalid value for Type, then when the type is not supported, we return a value of type Invalid, then it can be checked with:

v := ValueOf(unsupportedValue)

if v.Type() == stats.Invalid {
   ...
}

What do you think?

Also, if you have an example of stack trace for a panic that was triggered, and a code example of how this new API would be used, it could be helpful to determine what the best solution would be.

[collinvandyck]

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) }
	}

[achille-roussel]

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)
    }
}

[collinvandyck]

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?

[achille-roussel]

Sounds good to me 👍

[achille-roussel]

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}
    }
}

[collinvandyck]

ready for review

[achille-roussel]

Nice!