-
Notifications
You must be signed in to change notification settings - Fork 67
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
SQL: Set relevant errors to downstream #1159
Conversation
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.
I'd only mark the rows.Err() as downstream but lgtm otherwise.
data/sqlutil/sql.go
Outdated
@@ -22,7 +23,7 @@ import ( | |||
func FrameFromRows(rows *sql.Rows, rowLimit int64, converters ...Converter) (*data.Frame, error) { | |||
types, err := rows.ColumnTypes() | |||
if err != nil { | |||
return nil, err | |||
return nil, backend.DownstreamError(err) |
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.
I haven't seen this one erroring. I probably wouldn't change this to downstream. The same for rows.Columns
as it retruns the same errors.
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.
Updated! thank you @zoltanbedi
@@ -73,7 +74,7 @@ func FrameFromRows(rows *sql.Rows, rowLimit int64, converters ...Converter) (*da | |||
} | |||
|
|||
if err := rows.Err(); err != nil { | |||
return frame, err | |||
return frame, backend.DownstreamError(err) |
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.
I'm fine marking this as a downstream error.
Making some changes based on some errors being incorrectly marked in the MSSQL data source (that I believe can also be incorrectly marked in Postgres and MySQL also).
The errors that I've modified here are ones that I believe can only error due to downstream issues rather than the plugin. Please let me know if you disagree as I think determining which the ultimate source of an error is quite challenging.
With some follow up PR's to grafana/grafana this would fix some issues we're seeing atm.
If we don't want to mark all of these as downstream then I believe we should at least mark the
rows.Err()
error as downstream.