Pass warnings through on non-error responses

[jacksontj]

Signed-off-by: Thomas Jackson jacksontj.89@gmail.com

Without this we have 2 problems:

(1) we cannot differentiate an error from a warning from the API response body (as the Err() method is returning self always

(2) we can inadvertently layer these errors together when using NewErrorAPI

This PR addresses both.

[jacksontj]

On a related note, I've been trying to integrate this warnings behavior as a client and I have to say the behavior/interface is infuriating to use.

According to the upstream prom docs the warnings are meant to be non-fatal. If the response has a warning and no error we currently return an Error type which implements the error interface. This is a bit clunky for direct usage, but manageable. If you are layering methods this is completely untenable. In golang if the err is not nil methods generally don't pass the value back up. A basic case would look like:

func PassThrough() (interface{}, error) {
    v, err := DownstreamFunc()
    if err != nil {
        return nil, err
    }
    return v, nil

Since this client is returning a non-nil thing that is a warning (and implements erro), if you want to still get the value when there is a warning but not an error it requires changing all code you have written before -- and (worse) to change your coding habits moving forward (as this is very not idiomatic go). To show how messy this is an example of this passing-through would look like:

func PassThrough() (interface{}, error) {
    v, err := DownstreamFunc()
    if apiErr, ok := err.(api.Error); ok  {
        if apiErr.Err() != nil {
            return nil, apiErr
        }
    }
    return v, api.NewErrorAPI(err)

Assuming upstream plans on keeping this behavior (warnings are non-fatal) IMO we should change the signature to pass warning separate from error. As it stands you basically have to rewrite everything once and change passthrough funcs and future coding habits. If we were to change the return to (value, warnings, error) It would require the one rewrite of the caller of the client but avoid problems with layering/passthrough.

Thoughts?

cc @krasi-georgiev

[krasi-georgiev]

@jacksontj yep looks like this implementation make the use of error quite bad so yeah lets try with your original suggestion.

[jacksontj]

Ok, I'll try and get a pr together some time this week.

…

On Tue, Jun 11, 2019, 6:26 AM Krasi Georgiev ***@***.***> wrote: @jacksontj <https://github.com/jacksontj> yep looks like this implementation make the use of error quite bad so yeah lets try with your original suggestion. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#599?email_source=notifications&email_token=AAMLPHMVKJHTUYDTN2J7BN3PZ6RXNA5CNFSM4HWD4M7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXNDMJA#issuecomment-500839972>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMLPHNCIA6XTRNEPHCAJETPZ6RXNANCNFSM4HWD4M7A> .

[jacksontj]

@krasi-georgiev here is the change, for now I only wired up warnings to query/queryRange -- as they are the only ones that pass that information back. If these get added to others in the future it would be easy to add -- but I don't think thats planned.

[krasi-georgiev]

I am a bit confused by this. The queryTests don't have the inWarnings set anywhere so this will always be true test.inWarnings == nil

[jacksontj]

@krasi-georgiev true, I've added a couple cases that do so.

[krasi-georgiev]

but than you should also check the content right? testutil.Equals(t,test.inWarnings,warnings)

[krasi-georgiev]

and don't forget the full stop for the comments to follow the golang standards. I know it is annoying until you get used to it 😸

[jacksontj]

Updated again :)

[krasi-georgiev]

Thanks!

[verdverm]

This breaks a lot of downstream users of this library who have been relying on go get without a lock file

[jacksontj]

Unfortunately this was expected, upstream prometheus added these warnings and due to the semantics of those warnings we end up basically requiring this (more discussion in #562).

[jacksontj]

Personally I'm finding this Warnings thing a major pain to integrate as it complicates things quite a bit more (and it doesn't really help IMO that its not really a "warning" from prometheus).

[verdverm]

Legit, we are not using go ver mgmt, on us in my mind

…

On Thu, Jun 13, 2019, 11:54 PM Thomas Jackson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In api/prometheus/v1/api.go <#599 (comment)> : > // Query performs a query for the given time. - Query(ctx context.Context, query string, ts time.Time) (model.Value, api.Error) + Query(ctx context.Context, query string, ts time.Time) (model.Value, api.Warnings, error) Unfortunately this was expected, upstream prometheus added these warnings and due to the semantics of those warnings we end up basically requiring this (more discussion in #562 <#562>). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#599?email_source=notifications&email_token=AAKTQCB2GOBKOZQHOBVXDHDP2MXBLA5CNFSM4HWD4M7KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3RGOLA#discussion_r293667153>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKTQCEES7M6MHKBUOA36PDP2MXBLANCNFSM4HWD4M7A> .