From ac6bf4109bbf4925830c04c2cd7c33117a24a2a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Thu, 25 Jun 2020 22:18:57 +0300 Subject: [PATCH 1/4] Query: always return a string in the `lastError` field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 1 + pkg/query/storeset.go | 21 +++++++++++++++++++-- pkg/query/storeset_test.go | 29 ++++++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 848b14ab98..ca968c46a9 100644 --- a/CHANGELOG.md +++ b/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. +- []() Query: `/api/v1/stores` now guarantees to return a string in the `lastError` field ### Changed diff --git a/pkg/query/storeset.go b/pkg/query/storeset.go index b2db21dc05..6e38e28815 100644 --- a/pkg/query/storeset.go +++ b/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 } +// stringifiedError forces the error to be a string +// when marshalled into a JSON. +type stringifiedError struct { + originalErr error +} + +// MarshalJSON marshals the error into a string form. +func (e *stringifiedError) MarshalJSON() ([]byte, error) { + return json.Marshal(e.originalErr.Error()) +} + +// Error returns the original underlying error. +func (e *stringifiedError) Error() string { + return e.originalErr.Error() +} + type StoreStatus struct { Name string `json:"name"` LastCheck time.Time `json:"lastCheck"` - LastError error `json:"lastError"` + LastError stringifiedError `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 = stringifiedError{originalErr: err} if err == nil { status.LastCheck = time.Now() diff --git a/pkg/query/storeset_test.go b/pkg/query/storeset_test.go index 273a2e086c..b1630db9d5 100644 --- a/pkg/query/storeset_test.go +++ b/pkg/query/storeset_test.go @@ -5,6 +5,7 @@ package query import ( "context" + "encoding/json" "fmt" "math" "net" @@ -12,6 +13,7 @@ import ( "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 TestStringifiedError(t *testing.T) { + weirdError := &weirdError{originalErr: errors.New("test")} + properErr := &stringifiedError{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") +} From 3d55e25410e3e921b11b7ee200b7f8111c494d4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Thu, 25 Jun 2020 22:31:15 +0300 Subject: [PATCH 2/4] CHANGELOG: add PR number + remove whitespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Giedrius Statkevičius --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca968c46a9..f2a23919b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +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. -- []() Query: `/api/v1/stores` now guarantees to return a string in the `lastError` field +- [#2809](https://github.com/thanos-io/thanos/pull/2809) Query: `/api/v1/stores` now guarantees to return a string in the `lastError` field ### Changed From 3c3416e897b5e6b9241dd12bf2f06975bda630d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Thu, 25 Jun 2020 22:45:24 +0300 Subject: [PATCH 3/4] query/storeset: fix a subtle bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Giedrius Statkevičius --- pkg/query/storeset.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/query/storeset.go b/pkg/query/storeset.go index 6e38e28815..16f4119e9f 100644 --- a/pkg/query/storeset.go +++ b/pkg/query/storeset.go @@ -67,7 +67,7 @@ func (e *stringifiedError) Error() string { type StoreStatus struct { Name string `json:"name"` LastCheck time.Time `json:"lastCheck"` - LastError stringifiedError `json:"lastError"` + LastError *stringifiedError `json:"lastError"` LabelSets []storepb.LabelSet `json:"labelSets"` StoreType component.StoreAPI `json:"-"` MinTime int64 `json:"minTime"` @@ -527,7 +527,7 @@ func (s *StoreSet) updateStoreStatus(store *storeRef, err error) { status = *prev } - status.LastError = stringifiedError{originalErr: err} + status.LastError = &stringifiedError{originalErr: err} if err == nil { status.LastCheck = time.Now() From 1ac9338969036d1aff1dfb7f2c3e8304fd234e53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Giedrius=20Statkevi=C4=8Dius?= Date: Fri, 26 Jun 2020 19:02:33 +0300 Subject: [PATCH 4/4] Make changes according to Bartek's suggestions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Giedrius Statkevičius --- CHANGELOG.md | 2 +- pkg/query/storeset.go | 12 ++++++------ pkg/query/storeset_test.go | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2a23919b2..1c9b308045 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +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 +- [#2809](https://github.com/thanos-io/thanos/pull/2809) Query: `/api/v1/stores` now guarantees to return a string in the `lastError` field. ### Changed diff --git a/pkg/query/storeset.go b/pkg/query/storeset.go index 16f4119e9f..654cee494b 100644 --- a/pkg/query/storeset.go +++ b/pkg/query/storeset.go @@ -48,26 +48,26 @@ type RuleSpec interface { Addr() string } -// stringifiedError forces the error to be a string +// stringError forces the error to be a string // when marshalled into a JSON. -type stringifiedError struct { +type stringError struct { originalErr error } // MarshalJSON marshals the error into a string form. -func (e *stringifiedError) MarshalJSON() ([]byte, error) { +func (e *stringError) MarshalJSON() ([]byte, error) { return json.Marshal(e.originalErr.Error()) } // Error returns the original underlying error. -func (e *stringifiedError) Error() string { +func (e *stringError) Error() string { return e.originalErr.Error() } type StoreStatus struct { Name string `json:"name"` LastCheck time.Time `json:"lastCheck"` - LastError *stringifiedError `json:"lastError"` + LastError *stringError `json:"lastError"` LabelSets []storepb.LabelSet `json:"labelSets"` StoreType component.StoreAPI `json:"-"` MinTime int64 `json:"minTime"` @@ -527,7 +527,7 @@ func (s *StoreSet) updateStoreStatus(store *storeRef, err error) { status = *prev } - status.LastError = &stringifiedError{originalErr: err} + status.LastError = &stringError{originalErr: err} if err == nil { status.LastCheck = time.Now() diff --git a/pkg/query/storeset_test.go b/pkg/query/storeset_test.go index b1630db9d5..58813b2dce 100644 --- a/pkg/query/storeset_test.go +++ b/pkg/query/storeset_test.go @@ -795,9 +795,9 @@ func (e *weirdError) Error() string { return e.originalErr.Error() } -func TestStringifiedError(t *testing.T) { +func TestStringError(t *testing.T) { weirdError := &weirdError{originalErr: errors.New("test")} - properErr := &stringifiedError{originalErr: weirdError} + properErr := &stringError{originalErr: weirdError} storestatusMock := map[string]error{} storestatusMock["weird"] = weirdError storestatusMock["proper"] = properErr