Azure / azure-kusto-go

Azure Data Explorer (Kusto) SDK for Go
MIT License
58 stars 41 forks source link

Provide access to structured REST errors that are returned #12

Closed element-of-surprise closed 7 months ago

element-of-surprise commented 4 years ago

Originally a part of issue: https://github.com/Azure/azure-kusto-go/issues/10

Errors returned as part of Kusto can come in a variety of shapes. Some are errors generated by authorization systems, some are generated by Kusto, etc....

These errors may come in two forms

The JSON encoded may also come in different forms. Some are OneAPI errors, but off an earlier spec that is no longer valid or some other JSON encoded structure.

Currently, we simply try to look at the OneAPI errors we have seen and simply put them as wrapped errors inside Go's error type. This probably has limited use.

Go has a wide variety of error detection methods. However, these are all predicated on that the Go code knows what the errors are going to be. At this time, I don't think it is realistic to believe we can do that. We simply understand the severity of the issue and where the error comes from.

In those issues, we have error codes that can be introspected via the errors library.

But we do not have anything to allow dynamic introspection of the data when it is JSON encoded. This is being asked for.

Here is the original part of the request:

Additionally, the main issue is that the actual Kusto error response is basically lost. All that's returned from a Kusto call is an HTTP Error with a text string containing the http response body embedded inside it.

It appears as though Kusto returns a json struct for the error with code, message, @type, @context, etc. Shouldn't there be a formal Kusto error struct that's optionally set and returned with errors returned by calls to Kusto Go APIs ?
patrickbsf commented 4 years ago

I'd recommend to just followed 1.13+ standards for errors: https://blog.golang.org/go1.13-errors

Callers can then use .Is or .As as appropriate to pull out the 'wrapped' details when available.

element-of-surprise commented 4 years ago

Our errors package can do wrapped errors as 1.13. The problem is, unfortunately, more nuanced.

Wrapped errors and Go's error blogs assume that you have complete control of errors. Go has a lot of ways of expressing a particular error for introspection.

Wrapping provides the creator the option of returning multiple types of errors via wrapping.

So if you provide an error to the client such as (simplified example):

var ErrStream = "stream had a problem"

It can wrap a lower level error like: io.ErrNoProgress and you can do detection on the exact reason for the stream error via Is or As.

Today we provide error categories that can wrap other errors. Our error categories allow you to do first level interrogation on the error's class at the client level (is this an internal error, did I provide the wrong arguments, etc.....). This follows a model Pike established. What it doesn't allow it deeper introspection of the error returned by REST itself.

The reason we don't have deeper introspection of the REST error at this time is that client doesn't know what we are going to get from the server in order to categorize the error message. At this point, we simply are going to get some type of string from the REST endpoint. Without giving that error structure, it would be some generic error with no usefulness to the user.

These errors can come from multiple systems, each one can dynamically add error types and messages at any time, change formats or categorization of the error message.

There are different strategies we could explore, such as:

patrickbsf commented 4 years ago

I would certainly hope that the Kusto API is capable of parsing and handling Kusto specific error responses. Beyond that, it can certainly be made somewhat generic.

At a minimum, I should be able to interrogate the error for basic details. ie: here's a returned error (all as text).

Op(OpMgmt): Kind(KHTTPError): received unexpected HTTP code 400 Bad Request, response body:
{
    "error": {
        "code": "BadRequest",
        "message": "Request is invalid and cannot be executed.",
        "@type": "Kusto.Common.Svc.Exceptions.EntityAlreadyExistsException",
        "@message": "Entity 'bounce_mapping' of kind 'Csv Mapping' already exists.",
        "@context": {
            "timestamp": "2020-03-16T17:12:44.4263937Z",
            "serviceAlias": "  .....  ",
            "machineName": "KEngine000000",
            "processName": "Kusto.WinSvc.Svc",
            "processId": 3252,
            "threadId": 6652,
            "appDomainName": "Kusto.WinSvc.Svc.exe",
            "clientRequestId": "KGC.execute;f0ad2bf8-e762-4b3b-a602-8128b5339f80",
            "activityId": "d68a8b97-5acd-4163-aae6-031d2991ed7a",
            "subActivityId": "06affb62-baed-4f9c-82d8-ca42a10a7ff2",
            "activityType": "DN.AdminCommand.TableIngestionMappingCreateCommand",
            "parentActivityId": "a1dd307c-d2ed-4783-8c11-1926a50ef44a",
            "activityStack": "(Activity stack: [stripped some things that may contain PII ... ])"
        },
        "@permanent": true
    }
}

I should be able to get the error back and know it's an EntityAlreadyExistsException error for example.

element-of-surprise commented 4 years ago

Kusto itself seems to provide errors consistent with an older spec of OneAPI. Not all errors that I have received while writing this conform to that spec. In addition, there are wrapped errors as well in some of these messages when it conforms.

Just to clarify, I am not a member of Kusto dev, hence why I don't have these answers at my fingertip. Kusto dev is also located in a different local, so it takes a bit of time to get additional answers.

The Kusto error types in the messages seemed to be based on .Net errors from some library instead of their own REST defined errors.

We could try to define enumerated errors for detection, such as:

// KustoErr defines types of errors that the Kusto REST API returns.
type KustoErr int

func (k KustoError) isKustoErr()

const (
  KErrUnknown KustoErr= 0
  EntityAlreadyExistsException KustoErr = 1
  ...
)

// IsKustoErr indicates if Kusto's REST API returned an error of the following types.
func IsKustoErr(errorTypes []KustoErr) bool {...}

// CanRetry indicates if the error is transient or permanent.
func CanRetry(e error) bool {...}

// KustoErrMsg returns the Kusto REST API error if it was provided by the server.
func KustoErrMsg(err error) *KustoErrMsg {...}

// KustoMsg details an error message from Kusto's REST API. This can be extracted with KustoErrMsg().
type KustoMsg {
  Code string
  Message string
  Type string
  AtMessage string
  Context string
}

However, that method requires us to enumerate all the errors from the various libraries and keep them in sync. This would fail long term, non-automatic synchronization from non single sourced locations is always an uphill battle.

Matching against strings is an anti-pattern, thoughI recognize a lot of REST APIs like to do this (or stringly type everything).

I will talk to the backend devs, try to figure out what the best possible solution is.

patrickbsf commented 4 years ago

I'd be curious how this is handled in the other Kusto APIs? .Net / Python, etc. It seems surprising that what it returns is so loosely defined but it sounds like the Kusto REST API is passing the buck a bit.

element-of-surprise commented 4 years ago

In Python, looks like it just throws a general exception without any codes:

https://github.com/Azure/azure-kusto-python/blob/5fed55fdb25765d4b8143065a302cec0f1c5be5a/azure-kusto-data/azure/kusto/data/request.py#L626

I'll talk with the backed guys, see if we can improve this on this side better.

element-of-surprise commented 4 years ago

Talking to the dev group, there is no central repository of error codes. The errors are .Net errors spread out through multiple libraries. So there is no good way to enumerate the errors that we would receive.

This means for introspection purposes, we are looking at matching against a string type like: "Kusto.Common.Svc.Exceptions.EntityAlreadyExistsException"

That really means client users being reactive against errors from the backend because there will be no definitive list in the client.

So I'm going to put a pause on that for a minute while I ponder it a bit.

However, I think that "@permanent" could be immediately useful and easily implemented.

func CanRetry(err error) bool {...}

This could be implemented so that certain classes of errors we already implement (we do have error types already, just no sub types for REST) can let you know if the error can be retried. And for REST errors that have this field, we can use that value.

That probably helps with 80% of the cases, just knowing if the problem is transient.

element-of-surprise commented 4 years ago

New release with Retry(). Leaving this bug open as there are a few other enhancement I'm going to make here.

vladikbr commented 3 years ago

Are you going to make other enhancements here or should we just close this?

element-of-surprise commented 3 years ago

I'm considering a new errors package in the future. The OneError thing is like a moving target and the backend sometimes sends them and sometimes doesn't. I'm thinking of doing something simpler like I implemented in MSAL.

I think its fine to close this and I can open up another one that has something more well thought out than my one line comment above.

AsafMah commented 7 months ago

Finally fixed in 1.0.0-preview