Skip to content

Conversation

gotjosh
Copy link
Contributor

@gotjosh gotjosh commented Apr 1, 2020

What this PR does:

This commit changes which error we return on validation back to the
clients (Prometheus or Distributors).

The samples that do not pass validation at distributors and/or ingesters
now return the first validation error as opposed to the last.

Which issue(s) this PR fixes:
Abstract from a conversation in #2336

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

This commit changes which error we return on validation back to the
clients (Prometheus or Distributors).

The samples that do not pass validation at distributors and/or ingesters
now return the first validation error as opposed to the last.

Signed-off-by: gotjosh <josue@grafana.com>
@@ -357,7 +357,7 @@ func (i *Ingester) Push(ctx context.Context, req *client.WriteRequest) (*client.
return nil, fmt.Errorf("no user id")
}

var lastPartialErr *validationError
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TSDB blocks ingester already does this.

Signed-off-by: gotjosh <josue@grafana.com>
Signed-off-by: gotjosh <josue@grafana.com>
@gotjosh
Copy link
Contributor Author

gotjosh commented Apr 1, 2020

Hi everyone 👋,

A question and a statement from me.

  1. It feels like this behaviour should be documented somewhere. My vote goes to the Remote API section but please let me know if there's a better place for that.

  2. One of the reasons behind this change is that with the work being done in Allow Cortex to receive WriteRequest with Metadata. #2336 we'll also introduce metadata failure errors. Given I propose processing series/samples first, returning the last error (which would be metadata related) doesn't really make sense.

@pracucci
Copy link
Contributor

pracucci commented Apr 2, 2020

  1. It feels like this behaviour should be documented somewhere. My vote goes to the Remote API section but please let me know if there's a better place for that.

Yes, please. To me there's no strong need to do it within this PR. You can take a bit more time and work on a overall improvement of the "Cortex API" doc which include mentioning this behaviour.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I agree on this change and thanks for testing it. Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants