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 experimental/errorsource and remove experimental/errorsource/httpclient #1150

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ivanahuckova
Copy link
Member

@ivanahuckova ivanahuckova commented Nov 20, 2024

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 exported errorWithSourceImpl.

  • deprecates all methods/middlewares in experimental/errorource and instead uses backend error source stuff

Discussion points:

  • I have gotten rid off override params when calling DownstremError and PluginError but I am unsure about this.
  • Also previously had error in error source source and status, 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 have status.ErrorWithSource, which is what we want. All Error methods have been implemented for status.ErrorWithSource. The only thing that does not have is status 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.

@ivanahuckova ivanahuckova changed the title Deprecate experimental/errorsource and remove experimental/errorsource/httpclient Deprecate experimental/errorsource and remove experimental/errorsource/httpclient Nov 20, 2024
@ivanahuckova ivanahuckova marked this pull request as ready for review November 20, 2024 14:07
@ivanahuckova ivanahuckova requested review from a team as code owners November 20, 2024 14:08
Copy link
Member

@wbrowne wbrowne left a 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!

experimental/errorsource/error_source_middleware.go Outdated Show resolved Hide resolved
experimental/errorsource/error_source_middleware.go Outdated Show resolved Hide resolved
experimental/errorsource/error_source_middleware.go Outdated Show resolved Hide resolved
@@ -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"?$`)},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these changes unrelated?

Copy link
Member Author

@ivanahuckova ivanahuckova Nov 26, 2024

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? 🤔

Copy link
Member

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? 🤔

Copy link
Member Author

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 🙂.

Copy link
Member

@wbrowne wbrowne Nov 26, 2024

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 👍

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

Successfully merging this pull request may close these issues.

2 participants