Query: always return a string in the lastError field (#2809)

* Query: always return a string in the `lastError` field

While testing out the new React UI, I have found out that some errors
return empty dicts when marshalled into the JSON format. The response
looks like this:

```
{..., "lastError": {}}
```

And then, obviously, the UI fails to parse that. Add a
`stringifiedError` type with tests which ensures that we always get a
string.

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

* CHANGELOG: add PR number + remove whitespace

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

* query/storeset: fix a subtle bug

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

* Make changes according to Bartek's suggestions

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

Author: GiedriusS

CHANGELOG.md

@@ -19,6 +19,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
 - [#2728](https://github.com/thanos-io/thanos/pull/2728) Query: Fixed panics when using larger number of replica labels with short series label sets.
 - [#2787](https://github.com/thanos-io/thanos/pull/2787) Update Prometheus mod to pull in prometheus/prometheus#7414.
 - [#2807](https://github.com/thanos-io/thanos/pull/2807) Store: decreased memory allocations while querying block's index.
+- [#2809](https://github.com/thanos-io/thanos/pull/2809) Query: `/api/v1/stores` now guarantees to return a string in the `lastError` field.
 
 ### Changed
 

pkg/query/storeset.go

@@ -5,6 +5,7 @@ package query
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"sort"
 	"strings"
@@ -47,10 +48,26 @@ type RuleSpec interface {
 	Addr() string
 }
 
+// stringError forces the error to be a string
+// when marshalled into a JSON.
+type stringError struct {
+	originalErr error
+}
+
+// MarshalJSON marshals the error into a string form.
+func (e *stringError) MarshalJSON() ([]byte, error) {
+	return json.Marshal(e.originalErr.Error())
+}
+
+// Error returns the original underlying error.
+func (e *stringError) Error() string {
+	return e.originalErr.Error()
+}
+
 type StoreStatus struct {
 	Name      string             `json:"name"`
 	LastCheck time.Time          `json:"lastCheck"`
-	LastError error              `json:"lastError"`
+	LastError *stringError       `json:"lastError"`
 	LabelSets []storepb.LabelSet `json:"labelSets"`
 	StoreType component.StoreAPI `json:"-"`
 	MinTime   int64              `json:"minTime"`
@@ -510,7 +527,7 @@ func (s *StoreSet) updateStoreStatus(store *storeRef, err error) {
 		status = *prev
 	}
 
-	status.LastError = err
+	status.LastError = &stringError{originalErr: err}
 
 	if err == nil {
 		status.LastCheck = time.Now()

pkg/query/storeset_test.go

@@ -5,13 +5,15 @@ package query
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"math"
 	"net"
 	"testing"
 	"time"
 
 	"github.com/fortytw2/leaktest"
+	"github.com/pkg/errors"
 	"github.com/thanos-io/thanos/pkg/component"
 	"github.com/thanos-io/thanos/pkg/store"
 	"github.com/thanos-io/thanos/pkg/store/storepb"
@@ -660,7 +662,7 @@ func TestQuerierStrict(t *testing.T) {
 	testutil.Equals(t, 2, len(storeSet.stores), "two static clients must remain available")
 	testutil.Equals(t, curMin, storeSet.stores[staticStoreAddr].minTime, "minimum time reported by the store node is different")
 	testutil.Equals(t, curMax, storeSet.stores[staticStoreAddr].maxTime, "minimum time reported by the store node is different")
-	testutil.NotOk(t, storeSet.storeStatuses[staticStoreAddr].LastError)
+	testutil.NotOk(t, storeSet.storeStatuses[staticStoreAddr].LastError.originalErr)
 }
 
 func TestStoreSet_Update_Rules(t *testing.T) {
@@ -778,3 +780,28 @@ func TestStoreSet_Update_Rules(t *testing.T) {
 		})
 	}
 }
+
+type weirdError struct {
+	originalErr error
+}
+
+// MarshalJSON marshals the error and returns weird results.
+func (e *weirdError) MarshalJSON() ([]byte, error) {
+	return json.Marshal(map[string]string{})
+}
+
+// Error returns the original, underlying string.
+func (e *weirdError) Error() string {
+	return e.originalErr.Error()
+}
+
+func TestStringError(t *testing.T) {
+	weirdError := &weirdError{originalErr: errors.New("test")}
+	properErr := &stringError{originalErr: weirdError}
+	storestatusMock := map[string]error{}
+	storestatusMock["weird"] = weirdError
+	storestatusMock["proper"] = properErr
+	b, err := json.Marshal(storestatusMock)
+	testutil.Ok(t, err)
+	testutil.Equals(t, []byte(`{"proper":"test","weird":{}}`), b, "expected to get proper results")
+}