-
Notifications
You must be signed in to change notification settings - Fork 66
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 experimental/errorsource
and remove experimental/errorsource/httpclient
#1150
base: main
Are you sure you want to change the base?
Conversation
experimental/errorsource
and remove experimental/errorsource/httpclient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Awesome work - just a few minor things!
@@ -22,7 +22,7 @@ func TestParseInterval(t *testing.T) { | |||
{inp: "2w", duration: 2 * 168 * time.Hour}, | |||
{inp: "1M", duration: time.Duration(daysInMonth * 24 * int(time.Hour))}, | |||
{inp: "1y", duration: time.Duration(daysInYear * 24 * int(time.Hour))}, | |||
{inp: "invalid-duration", err: regexp.MustCompile(`^time: invalid duration "?invalid-duration"?$`)}, | |||
{inp: "invalid-duration", err: regexp.MustCompile(`time: invalid duration "?invalid-duration"?$`)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes unrelated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is related. Because in status_source package, for ErrorWithErrorSource.Error()
method, we are prepending error source: https://github.com/grafana/grafana-plugin-sdk-go/blob/main/experimental/status/status_source.go#L86C1-L88C2
func (e errorWithSourceImpl) Error() string {
return fmt.Errorf("%s error: %w", e.source, e.err).Error()
}
This is a change compared to error_source, where we only returned the error https://github.com/grafana/grafana-plugin-sdk-go/blob/main/experimental/errorsource/error_source.go#L22
// Error implements the interface
func (r Error) Error() string {
return r.err.Error()
}
I could have changed it to {inp: "invalid-duration", err: regexp.MustCompile(^downstream error: time: invalid duration "?invalid-duration"?$)}
as well.
But also what do you think about prepending error and should we not do it? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah 😄 I thought since the function didn't change the test shouldn't, but I didn't realize before it was a different path where it just showed the error.
But also what do you think about prepending error and should we not do it? 🤔
If you think it's necessary. Otherwise I wonder will the logger catch it anyway and put the source as a label? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it's necessary.
I don't think it is necessary. If you don't think it is necessary, we can remove 🙂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure let's keep it simple for now (and remove) and can always come back to improve if necessary 👍
Co-authored-by: Will Browne <[email protected]>
Co-authored-by: Will Browne <[email protected]>
Co-authored-by: Will Browne <[email protected]>
This PR:
removes
experimental/errorsource/httpclient
. This was used only in 4 data sources and I have removed the usage in all of them (Elasticsearch, Opensearch, Wavefront, Mongodb). If needed, I can move this to separate PR.exports
ErrorWithSource
that is basically renamed and exportederrorWithSourceImpl
.deprecates all methods/middlewares in
experimental/errorource
and instead uses backend error source stuffDiscussion points:
DownstremError
andPluginError
but I am unsure about this.source
andstatus
, but I don't think we were really using a status. I got rid of it, but open to discussion.From CI:
incompatible changes in github.com/grafana/grafana-plugin-sdk-go/experimental/errorsource
Error: changed from Error to github.com/grafana/grafana-plugin-sdk-go/experimental/status.ErrorWithSource
New: changed from func(error, github.com/grafana/grafana-plugin-sdk-go/experimental/status.Source, github.com/grafana/grafana-plugin-sdk-go/backend.Status) Error to func(error, github.com/grafana/grafana-plugin-sdk-go/experimental/status.Source, github.com/grafana/grafana-plugin-sdk-go/backend.Status) github.com/grafana/grafana-plugin-sdk-go/experimental/status.ErrorWithSource
SourceError: changed from func(github.com/grafana/grafana-plugin-sdk-go/experimental/status.Source, error, bool) Error to func(github.com/grafana/grafana-plugin-sdk-go/experimental/status.Source, error, bool) github.com/grafana/grafana-plugin-sdk-go/experimental/status.ErrorWithSource
All of these are saying that instead of
Error
we now havestatus.ErrorWithSource
, which is what we want. AllError
methods have been implemented forstatus.ErrorWithSource
. The only thing that does not have isstatus backend.Status
, but that was not used/exported anywhere.incompatible changes github.com/grafana/grafana-plugin-sdk-go/experimental/errorsource/httpclient
package removed
Tis is done on purpose. This package was experimental and before removing, we ensured all data sources that were using it were migrated to not use it anymore.