Add storage.Warnings to client #562

jacksontj · 2019-04-29T20:51:11Z

closes #560

This adds storage.Warnings to the error return of Query and QueryRange. Unfortunately the client.Do() method interface makes this a bit difficult to implement.

So either we go down this path (where the specific error is a known type people should check for) or we need to change the Do() methods' interface to return a separate "warning" error.

jacksontj · 2019-04-30T14:33:25Z

Cc @krasi-georgiev

krasi-georgiev

I quick note now, but will do a full review soon.

api/prometheus/v1/api.go

jacksontj · 2019-04-30T17:30:10Z

@krasi-georgiev the main question is what the interface should be. Right now I'm wrapping the Warnings into a specific error type, which is different than the prom internals, but changes less of this client. Alternatively I can make the return of Do() add an additional return type of Warnings and have it be nil for everyone else -- either way works for me.

jacksontj · 2019-05-03T16:49:25Z

@krasi-georgiev bump?

krasi-georgiev · 2019-05-05T16:12:05Z

will review in 1-2 days.

krasi-georgiev · 2019-05-06T07:24:49Z

I think it should be something like:

type Error struct {
	Type   ErrorType
	Msg    string
	Detail string
	Warnings []string
}

jacksontj · 2019-05-06T22:56:37Z

@krasi-georgiev to clarify, you want to replace the error in the return with *Error? Or are you suggesting that Warnings be this new Error struct?.

From the upstream behavior warnings != error, so it seems incorrect for the client to be merging them. And if we decided that we wanted to then we'd definitely need a mechanism for the caller to determine what is an error vs a warning. In this proposed error struct thats a bit unclear. I'm also unclear what detail in the error would be, as the response struct (as taken from prometheus' api server) is:

type response struct {
	Status    status      `json:"status"`
	Data      interface{} `json:"data,omitempty"`
	ErrorType errorType   `json:"errorType,omitempty"`
	Error     string      `json:"error,omitempty"`
	Warnings  []string    `json:"warnings,omitempty"`
}

krasi-georgiev · 2019-05-07T09:09:17Z

maybe you are right lets try to keep these separately to be similiar to upstream.

in that case why not keep it seperately starting from the client?

func (c apiClient) Do(ctx context.Context, req *http.Request) (*http.Response, []byte,Warnings,error) {

api/prometheus/v1/api.go

jacksontj · 2019-05-08T03:12:56Z

@krasi-georgiev It isn't split right now because I hadn't heard back on the plan -- #562 (comment)

So it sounds like we want to keep warning/error separate all the way down? If so I can definitely do that.

jacksontj · 2019-05-08T03:17:35Z

#562 (comment)

All the way down to the HTTP client doesn't quite work, at this client level all we do is return a resp, body, and error -- at this level we haven't unmarshaled the response enough to know if there are warnings.

api/prometheus/v1/api.go

jacksontj · 2019-05-08T14:52:08Z

@krasi-georgiev here's it with warnings split all the way down

krasi-georgiev

hm , now I see what you meant by typecasting the error type.

Now when it is completely separated it pollutes all other methods that don't really need it and end up with _, _, _, err = h.client.Do(ctx, req)

Do you think that maybe we can implement some custom API error interface like
something like

type ErrorAPI interface{
	Error()
	Warning()
}

api/prometheus/v1/api_test.go

jacksontj · 2019-05-10T04:41:52Z

For the error response, if we want to have a single return type, IMO we should go back to what I had before where we have a specific Warnings struct that implements error. This way we the interface remains the same and the client can decide if they want to treat warnings differently or not. IMO this is how upstream prom should have dealt with it -- but do realize that will make the client interface different from the internal interface.

krasi-georgiev · 2019-05-10T07:57:37Z

For the error response, if we want to have a single return type, IMO we should go back to what I had before where we have a specific Warnings struct that implements error.

do you mean with typecasting? Do you think we can avoid typecasting by using an interface?

but do realize that will make the client interface different from the internal interface.

I don't think this is a problem.

jacksontj · 2019-05-10T14:28:30Z

Yes I mean with typecasting. Otherwise if they want an error or warning separately the client would have to convert out custom interface or stuck over. So what I had before was a `Warnings` type that implemented the error interface, so the caller could check if it's a warning

…

On Fri, May 10, 2019, 12:57 AM Krasi Georgiev ***@***.***> wrote: For the error response, if we want to have a single return type, IMO we should go back to what I had before where we have a specific Warnings struct that implements error. do you mean with typecasting? Do you think we can avoid typecasting by using an interface? but do realize that will make the client interface different from the internal interface. I don't have a problem with that. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#562 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMLPHLV4DWTCBIONZAO2NDPUUTHHANCNFSM4HJGU2TQ> .

krasi-georgiev · 2019-05-13T14:16:24Z

I was thinking something like this:

type ErrorAPI struct {
	error    string
	warning  string
}

func (w *ErrorAPI) Error() string {
	return w.error
}

func (w *ErrorAPI) Warning() string {
	return w.warnings
}

type Error interface {
	error
	Warning() string
}

and than

func (c *httpClient) Do(ctx context.Context, req *http.Request) (*http.Response, []byte, Error) {

so that the caller can use err.Error() and err.Warning() as needed.

jacksontj · 2019-05-15T05:00:27Z

@krasi-georgiev Updated with a single interface for the error, some small cleanup required; but since we are going back and forth a bunch I figured I'll let you do an initial review before I spend the time cleaning it up all the way.

krasi-georgiev · 2019-05-15T07:27:04Z

yes, I think this is the best approach so far so go ahead with the cleanup and ping me when ready for a review.

jacksontj · 2019-05-15T15:21:47Z

@krasi-georgiev Cleanup done, hopefully thats enough to get this in :)

api/client.go

krasi-georgiev · 2019-05-15T18:32:25Z

api/client.go

+
+type Error interface {
+	error
+	Err() error


Why the need for Err() and Error()?

Err() returns the error itself. This is important if you want to check if the error is something in particular. This is especially important as we have been returning the err directly until now. So old code that did something like:

ret, err := client.DoSomething() if err != nil { return nil, err }

would now be able to do something like

ret, err := client.DoSomething() if err != nil && err.Err() != nil { return nil, err.Err() }

hm I still don't see the need for this.
Can you give an example for how and where it is needed and why it can't be achieved by just using err.Error() ?

even without err.Err you can still have

if err != nil && err.Error() != "" { return nil, err.Error() }

err.Error() returns a string, if you need to check that is a specific error type or something then it is not sufficient.

For example, when we do a client get we could get back an error from the HTTP library. In the client it looks like so:

_, body, apiErr := h.client.Do(ctx, req) if apiErr != nil { return AlertsResult{}, apiErr }

So if I wanted to handle the specific error type of http.ProtocolError I'd need to do something like:

if err != nil { if typedErr, ok := err.Err().(*http.ProtocolError); ok { // do something else } return nil, err }

If we only have the string representation the people are forced to resort to string matching, which is a bad plan :)

Yes I agree. Lets keep just

// Error ..... type Error interface { // Error ...... Error() error // Warnings returns a list of warnings Warnings() []string }

There are still few places without comments the go-lint complains about it.

As I'm thinking about this more, I think we want to stick with:

type Error interface{ error Err() error Warnings() []string }

and change the implementations of Error() (the string return) to check the warnings. This way its more-or-less backwards compatible -- although it would mean that warnings are errors (if you wanted to skip them you'd have to check the Err() method). This would make it more compatible, and a little less clunky.

If we want the default behavior to be that people have to check if it was an error or warning then I think going to some interface that doesn't adhere to the error interface would be okay -- it will just be completely incompatible with all client usage.

Yes you are right lets keep as is for now and will refactor if needed.

@krasi-georgiev to clarify, do you want me to change the Error() implementation to return something if there was a warning (e.g. if no error but warnings, return the warnings concated somehow)?

@krasi-georgiev bump :)

Sorry, I am at kubecon so will be slow on replies.
I meant to keep it as

type Error interface{ error Err() error Warnings() []string }

api/client.go

api/prometheus/v1/api_test.go

jacksontj · 2019-05-15T19:25:29Z

@krasi-georgiev PR updated to address comments

krasi-georgiev

I think these are the last nits I could find 😺

You are right about the interface lets keep as is for now and will refactor if needed.

api/client.go

krasi-georgiev · 2019-05-17T09:38:07Z

api/client.go

+
+type Error interface {
+	error
+	Err() error


Yes you are right lets keep as is for now and will refactor if needed.

api/prometheus/v1/api_test.go

krasi-georgiev · 2019-05-20T19:17:08Z

LGTM
@beorn7 any comment from your side?

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com> Fixes prometheus#560

jacksontj · 2019-05-21T14:36:33Z

@krasi-georgiev squashed, should be ready for merge.

beorn7 · 2019-05-21T22:31:23Z

Didn't check all the details, but no general remarks from my side. I trust you to do the right thing in detail.

jacksontj force-pushed the issue_560 branch 3 times, most recently from e45a706 to a412d02 Compare April 30, 2019 17:03

krasi-georgiev reviewed Apr 30, 2019

View reviewed changes

api/prometheus/v1/api.go Outdated Show resolved Hide resolved

krasi-georgiev reviewed May 7, 2019

View reviewed changes

api/prometheus/v1/api.go Outdated Show resolved Hide resolved

krasi-georgiev reviewed May 8, 2019

View reviewed changes

api/prometheus/v1/api.go Show resolved Hide resolved

jacksontj force-pushed the issue_560 branch 2 times, most recently from 4175e10 to 13dc6f7 Compare May 8, 2019 14:52

krasi-georgiev reviewed May 9, 2019

View reviewed changes

api/prometheus/v1/api_test.go Outdated Show resolved Hide resolved

api/prometheus/v1/api_test.go Outdated Show resolved Hide resolved

jacksontj force-pushed the issue_560 branch from f14e079 to ae626b6 Compare May 15, 2019 15:21

krasi-georgiev reviewed May 15, 2019

View reviewed changes

krasi-georgiev reviewed May 17, 2019

View reviewed changes

Add storage.Warnings to client

d5f3c8d

Signed-off-by: Thomas Jackson <jacksontj.89@gmail.com> Fixes prometheus#560

jacksontj force-pushed the issue_560 branch from 318fac4 to d5f3c8d Compare May 21, 2019 14:36

beorn7 merged commit b46e6ec into prometheus:master May 21, 2019

This was referenced Jun 14, 2019

Pass warnings through on non-error responses #599

Merged

Add "warnings" from prometheus upstream jacksontj/promxy#168

Closed

Add "warnings" from prometheus upstream jacksontj/promxy#169

Closed

jacksontj mentioned this pull request Nov 25, 2019

Pass warning back from response so we can tell if there is a problem with prometheus #648

Closed

Add storage.Warnings to client #562

Add storage.Warnings to client #562

Conversation

jacksontj commented Apr 29, 2019 • edited by krasi-georgiev Loading

jacksontj commented Apr 30, 2019

krasi-georgiev left a comment

Choose a reason for hiding this comment

jacksontj commented Apr 30, 2019

jacksontj commented May 3, 2019

krasi-georgiev commented May 5, 2019

krasi-georgiev commented May 6, 2019

jacksontj commented May 6, 2019

krasi-georgiev commented May 7, 2019

jacksontj commented May 8, 2019

jacksontj commented May 8, 2019

jacksontj commented May 8, 2019

krasi-georgiev left a comment

Choose a reason for hiding this comment

jacksontj commented May 10, 2019

krasi-georgiev commented May 10, 2019 • edited Loading

jacksontj commented May 10, 2019 via email

krasi-georgiev commented May 13, 2019

jacksontj commented May 15, 2019

krasi-georgiev commented May 15, 2019

jacksontj commented May 15, 2019

Choose a reason for hiding this comment

jacksontj May 15, 2019 • edited Loading

Choose a reason for hiding this comment

krasi-georgiev May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacksontj commented May 15, 2019

krasi-georgiev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krasi-georgiev commented May 20, 2019

jacksontj commented May 21, 2019

beorn7 commented May 21, 2019

jacksontj commented Apr 29, 2019 •

edited by krasi-georgiev

Loading

krasi-georgiev commented May 10, 2019 •

edited

Loading

jacksontj May 15, 2019 •

edited

Loading

krasi-georgiev May 15, 2019 •

edited

Loading

krasi-georgiev left a comment •

edited

Loading