grafana / grafana-plugin-sdk-go

A Go SDK for building backend plugins for Grafana
Apache License 2.0
219 stars 66 forks source link

Deprecate and remove experimental error source middleware #1105

Closed ivanahuckova closed 15 hours ago

ivanahuckova commented 1 month ago

From https://github.com/grafana/grafana-plugin-sdk-go/pull/1106#pullrequestreview-2360581383:

marefr commented 1 month ago

Regarding "We should add methods/options that add error source, but will not override it if it exists".

@ivanahuckova https://github.com/grafana/grafana-plugin-sdk-go/pull/1106#discussion_r1795449882:

Do we want to have a similar logic here from experimental/errorsource and let users decide if they want to override the error source in case it exists? https://github.com/grafana/grafana-plugin-sdk-go/blob/main/experimental/errorsource/error_source.go#L52. This is helpful when we (plugin developers) want to set error source, but only if it does not exist. Not sure if we should modify this or if it would be worth creating a new method.

// SourceError returns an error with the source
// If source is already defined, it will return it, or you can override
func SourceError(source backend.ErrorSource, err error, override bool) Error {
  var sourceError Error
  if errors.As(err, &sourceError) && !override {
      return sourceError // already has a source
  }
  return Error{
      source: source,
      err:    err,
  }
}

@ivanahuckova https://github.com/grafana/grafana-plugin-sdk-go/pull/1106#discussion_r1796609570:

I looked into this further and I think we should avoid overriding the status if it already exists. I reviewed the usage of [errorsource.DownstreamError() which lets you decide whether or not to override the source](https://github.com/search?q=org%3Agrafana%20.DownstreamError(&type=code), and in all cases, plugin developers choose not to override it. Since we are introducing this new method that might be used frequently, I'd suggest not overriding the status if it already exists. Instead, we could create a method like DownstreamErrorWithOverride for the rare cases where users want to override the status.

@marefr https://github.com/grafana/grafana-plugin-sdk-go/pull/1106#discussion_r1796721940:

First, this is not really a new function. And for now the experimental errorsource package are mostly used, but planned to be removed soon.

The risk of not overriding is that we drop important errors/context/information. Further, if error already contains a downstream source and you call DownstreamError with an error with a plugin source and not override we might drop what is actually a real plugin error/problem. Not sure what the approach should be here, but I would be in favor of the current way just overriding. In the opposite way I guess you want to override though 😱

So maybe the logic needs to

  • check if incoming error has plugin source it will override a downstream source
  • check if incoming error has downstream source and it will not override a plugin source, but we might be able to unwrap the error to not lose the details or something.
marefr commented 1 month ago

Reg "Deprecate experimental/errorsource and point plugin developers use non-experimental versions of functions." and "We should probably export errorWithSourceImpl and use it in experimental/errorsource instead of Error to ensure that all methods are working as intended with a new middleware. Specifically we need Response to work." we basically want to