-
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
Error source HTTP client middleware #1106
Conversation
420d538
to
b5e125e
Compare
b5e125e
to
bc0110f
Compare
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.
Awesome cleanup! Left some comments.
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! Things that we need to improve in follow up:
- We should probably export
errorWithSourceImpl
and use it inexperimental/errorsource
instead ofError
to ensure that all methods are working as intended with a new middleware. Specifically we needResponse
to work. - We should add methods/options that add error source, but will not override it if it exists
- Deprecate
experimental/errorsource
and point plugin developers use non-experimental versions of functions. - In
pkg/infra/httpclient/httpclientprovider/http_client_provider.go
we need to add ErrorSource middleaware so built in plugin case use it. Done in Plugins: Auto instrumentation improvements grafana#94193
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.
Left couple more comments and questions
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
Alternative to #1102
Moves
backend.ErrorSource
related implementation to a new packagestatus
. This tries to keep backward compatibility by keeping type aliases in thebackend
package referencing the newstatus
package.The main motivation behind this change is that having the
backend.ErrorSource
makes it basically impossible for any other package to make use of this if that package is imported by thebackend
package since it creates a circular dependency.Seems like
gorelease
doesn't agree these type alias changes are compatible. In my head they should be 🤔