grafana-plugin-sdk-go icon indicating copy to clipboard operation
grafana-plugin-sdk-go copied to clipboard

Improve default logic and error handling with FillMissing

Open kylebrandt opened this issue 5 years ago • 0 comments

Copied from https://github.com/grafana/grafana/pull/27054:

Converted to draft, want to think about changing the SDK...

In LongToWide, we call GetMissing.

```go
LongToWide(longFrame *Frame, fillMissing *FillMissing) (*Frame, error) {
// GetMissing returns the value to be filled for a missing row field.
func GetMissing(fillMissing *FillMissing, field *Field, previousRowIdx int) (interface{}, error) {
	if fillMissing == nil {
		return nil, fmt.Errorf("fill missing is disabled")
	}
	var fillVal interface{}
	switch fillMissing.Mode {
	case FillModeNull:
	//	fillVal = nil
	case FillModeValue:
		convertedVal, err := float64ToType(fillMissing.Value, field.Type())
		if err != nil {
			return nil, err
		}
		fillVal = convertedVal
	case FillModePrevious:
		// if there is no previous value
		// the value will be null
		if previousRowIdx >= 0 {
			fillVal = field.At(previousRowIdx)
		}
	}
	return fillVal, nil
}

But we are ignoring the error when we call this. However, due to the way we call it, the default behavior will be to have FillModeNull.

For example:

field.Extend(1)
fillVal, err := GetMissing(fillMissing, field, wideFrameRowCounter-1)
if err == nil {
	wideFrame.Set(wideFrameIdx, wideFrameRowCounter, fillVal)
}

Extend will be zero-value allocated, so we do not need to call set. So that is why passing nil to functions like LongToWide is the same as FillModeNull. However, this results in us ignoring the possible error from the call to float64ToType within GetMissing.

This behavior is also odd, because if you pass a non-nil FillMissing struct to LongToWide but do not specify the Mode, the default Mode will be FillPrevious, which is probably not a good default.

So:

  • [ ] drop the error if the FillMissing is null on GetMissing, and change it to have the same behavior as FillModeNull
  • [ ] Change the enum so FillModeNull is first, so an empty struct is the same as nil for get missing
  • [ ] Look int float64Type's possible errors. Either return an error in funcs like LongToWide, or add a frame warning. Probably warning/log to reduce risk of breaking changes to users.
  • [ ] Consider unexporting GetMissing by looking to see if it called outside the sdk.
  • [ ] Update LongToWide func doc to explain FillMissing.

kylebrandt avatar Aug 19 '20 14:08 kylebrandt