Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate and remove experimental error source middleware #1105

Closed
ivanahuckova opened this issue Oct 9, 2024 · 2 comments
Closed

Deprecate and remove experimental error source middleware #1105

ivanahuckova opened this issue Oct 9, 2024 · 2 comments
Assignees

Comments

@ivanahuckova
Copy link
Member

ivanahuckova commented Oct 9, 2024

From #1106 (review):

  • Deprecate experimental/errorsource and point plugin developers use non-experimental versions of functions.
  • 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 should add methods/options that add error source, but will not override it if it exists
@marefr
Copy link
Contributor

marefr commented Oct 22, 2024

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

@ivanahuckova #1106 (comment):

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 #1106 (comment):

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 #1106 (comment):

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
Copy link
Contributor

marefr commented Oct 22, 2024

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

  • deprecate the usage of the experimental/errorsource middleware due to Errorsource's RoundTripper returns custom error alongside response #846
  • move most of the experimental/errorsource functions (the ones used/needed) to the backend package
  • deprecate the experimental/errorsource.Error type and move it to experimental/status package and replace the errorWithSourceImpl

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

No branches or pull requests

2 participants