Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jun 30, 2020

Conversation

GiedriusS
Copy link
Member

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 [email protected]

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Add a small, new type which ensures that we always get strings in the lastError field of /api/v1/stores.

Verification

Unit tests.

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 <[email protected]>
Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS GiedriusS marked this pull request as ready for review June 25, 2020 19:31
@GiedriusS
Copy link
Member Author

cc @prmsrswt

Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS GiedriusS requested a review from squat June 26, 2020 08:20
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, mod small comments (:

BTW @prmsrswt feel free to review, you review matters! 🤗

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/query/storeset.go Outdated Show resolved Hide resolved
Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! this looks like a very clean solution.

@bwplotka bwplotka merged commit 7463266 into thanos-io:master Jun 30, 2020
paulfantom added a commit to paulfantom/thanos that referenced this pull request Jul 9, 2020
* upstream/release-0.14: (46 commits)
  Cut release v0.14.0-rc.1 (thanos-io#2853)
  Query: correctly marshal errors to JSON and ignore if nil (thanos-io#2848)
  ci: Manually download promu in crossbuild stage (thanos-io#2828)
  Cut release v0.14.0-rc.0 (thanos-io#2826)
  Soft cut changelog on master to indicate v0.14.0 being in progress (thanos-io#2824)
  Update ThanosReceiveNoUpload to select sum == 0 (thanos-io#2819)
  receive: Added more observability, fixed leaktest, to actually check leaks ): (thanos-io#2817)
  Query: always return a string in the `lastError` field (thanos-io#2809)
  Added missing CHANGELOG entry for PR 2613 (thanos-io#2820)
  receive: Fixed small options race; Removed unused StartTime feature. (thanos-io#2816)
  go.mod: Bump Prometheus to current latest (thanos-io#2814)
  Implement CLI Flags page in React UI (thanos-io#2796)
  Improve ThanosReceiveNoUpload to only alert on current instances
  store: Preallocate output buffer when encoding postings. (thanos-io#2812)
  compact: introduce flag --block-viewer.global.sync-block-interval (thanos-io#2752)
  docs: compact: add blurb about how retention policy works (thanos-io#2808)
  Reduced memory allocations in readIndexRange() (thanos-io#2807)
  ui: Add Stores page to React UI (thanos-io#2754)
  Added Kemal to Maintainer Role; Kemal is volounteering to be next release shephard (thanos-io#2804)
  proposal: Add scalable rule storage proposal (thanos-io#2661)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants