Set HTTP timeouts as downstream errors

[wbrowne]

Fixes #1064

[marefr]

We might need similar checks around here as well?

grafana-plugin-sdk-go/backend/data_adapter.go

Line 50 in 4cba513

if isCancelledError(r.Error) {

[wbrowne]

We might need similar checks around here as well?

grafana-plugin-sdk-go/backend/data_adapter.go

Line 50 in 4cba513

if isCancelledError(r.Error) {

Yeah so what should the behaviour be in this case:

• If HTTP timeout error for DataResponse, set downstream error source and return RequestStatusError?
• If HTTP timeout error from adapter, return RequestStatusError?

[marefr]

• If HTTP timeout error for DataResponse, set downstream error source and return RequestStatusError?

Yeah I guess so

• If HTTP timeout error from adapter, return RequestStatusError?

Yes and let the error wrapper handle that

[marefr]

LGTM 🎉

[xnyo]

LGTM!

[wbrowne]

@ivanahuckova Would you like to take this for a spin to make sure it's resolving the issue you've experienced?

[ivanahuckova]

Works like a charm ✨ THANK YOU!!

Could we also add this to api server instrumentation? 😇

[wbrowne]

@ivanahuckova Thank you! I think we may have also solved this one with this change too BTW:

• mark DeadlineExceeded desc = context deadline exceeded as downstream errors

Let me know if it's still an issue if you can 🙏

Could we also add this to api server instrumentation? 😇

If the error is returned from the handler, this is the classic problem of not having enough information on the other side since it's stripped over the wire. IIUC we'd need to add more to the plugin response (either directly to the protobuf) or some gRPC metadata that would indicate the appropriate status source.

For a data response error though, maybe we should be setting the error source on the response over here if it's not already set IE

if isCancelledError(r.Error) {
+  r.ErrorSource = ErrorSourceDownstream
  hasCancelledError = true
}
if isHTTPTimeoutError(r.Error) {
+  r.ErrorSource = ErrorSourceDownstream
  hasHTTPTimeoutError = true
}

It's probably safer to only do this if not already set. But that should then use the appropriate status source (would need to confirm though).

The more we touch here though in its current state, the more brittle it all feels 😬 Instrumenting the same thing across client and server is tricky.

Anything to add or correct @marefr?

[ivanahuckova]

mark DeadlineExceeded desc = context deadline exceeded as downstream errors
Let me know if it's still an issue if you can 🙏

I will try on friday

The more we touch here though in its current state, the more brittle it all feels 😬 Instrumenting the same thing across client and server is tricky.

Yeah, I agree. One solution could be to fully focus on updating the SDK in all plugins and skip the API server metrics for now. This would help resolve all the issues. 🤔 I initially wanted us to use the API server metrics until all plugins had updated their SDKs, but I've been realizing that using it temporarily requires too much effort. Instead, it seems we should just concentrate on updating the SDK in the plugins.

[marefr]

For a data response error though, maybe we should be setting the error source on the response over here if it's not already set IE

Isn't it what we do here

grafana-plugin-sdk-go/backend/data_adapter.go

Lines 69 to 90 in e1e82c0

	if hasCancelledError {
	if err := WithDownstreamErrorSource(ctx); err != nil {
	return RequestStatusError, fmt.Errorf("failed to set downstream status source: %w", errors.Join(innerErr, err))
	}
	return RequestStatusCancelled, nil
	}
	if hasHTTPTimeoutError {
	if err := WithDownstreamErrorSource(ctx); err != nil {
	return RequestStatusError, fmt.Errorf("failed to set downstream status source: %w", errors.Join(innerErr, err))
	}
	return RequestStatusError, nil
	}
	// A plugin error has higher priority than a downstream error,
	// so set to downstream only if there's no plugin error
	if hasDownstreamError && !hasPluginError {
	if err := WithDownstreamErrorSource(ctx); err != nil {
	return RequestStatusError, fmt.Errorf("failed to set downstream status source: %w", errors.Join(innerErr, err))
	}
	return RequestStatusError, nil
	}

and what we partially fixed in this PR? 🤔

Something that might be a potential problem is that we don't check here if it's not a plugin error

grafana-plugin-sdk-go/backend/data_adapter.go

Lines 69 to 81 in e1e82c0

	if hasCancelledError {
	if err := WithDownstreamErrorSource(ctx); err != nil {
	return RequestStatusError, fmt.Errorf("failed to set downstream status source: %w", errors.Join(innerErr, err))
	}
	return RequestStatusCancelled, nil
	}
	if hasHTTPTimeoutError {
	if err := WithDownstreamErrorSource(ctx); err != nil {
	return RequestStatusError, fmt.Errorf("failed to set downstream status source: %w", errors.Join(innerErr, err))
	}
	return RequestStatusError, nil
	}

We do that for hasDownstreamError
https://github.com/grafana/grafana-plugin-sdk-go/blob/e1e82c05cc52e8fb712dbf581a63e6f96f04965c/backend/data_adapter.go#L83C3-L85C45

[marefr]

Nevermind I think I see what you mean now. If there's a data response error and there's no ErrorSource set we currently default to plugin source, right?

[marefr]

I'll have a look at this and the new tasks added to #1050

[wbrowne]

Nevermind I think I see what you mean now. If there's a data response error and there's no ErrorSource set we currently default to plugin source, right?

Exactly!

I'll have a look at this and the new tasks added to #1050

Awesome - thank you 👍

[marefr]

PTAL #1066

[ivanahuckova]

I think it also solves also DeadlineExceeded error marking as downstream so marking that in #1050 as resolved.