Autoinstrumentation improvements

[marefr]

Follow up from #1063 and #1063 (comment)

Changes:

• Logs partial data response errors in a similar way as grafana does.
• Clean up code/logic for error source and request status in regards to query data.
• Fix a flaky test noted when running mage testRace

[marefr]

This is iterating the data responses and we do that below as well, but feels like much cleaner code doing it this way

[ivanahuckova]

It looks like that check if resp == nil || len(resp.Responses) == 0 { was needed also for code bellow, because now, if we return nil response, we get panic in for refID, r := range resp.Responses code

[xnyo]

That's correct, but I still get a separate panic even after adding back the same if resp == nil || len(resp.Responses) == 0 { check.

This time the panic is here:

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

Line 109 in e1e82c0

return ToProto().QueryDataResponse(resp)

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

Line 197 in 6158ec5

Responses: make(map[string]*pluginv2.DataResponse, len(res.Responses)),

I also get it on the latest main:

diff --git a/backend/data_adapter_test.go b/backend/data_adapter_test.go
index 57fd761..70423bb 100644
--- a/backend/data_adapter_test.go
+++ b/backend/data_adapter_test.go
@@ -180,6 +180,11 @@ func TestQueryData(t *testing.T) {
                                },
                                expErrorSource: ErrorSourcePlugin,
                        },
+                       {
+                               name:              "nil queryDataResponse",
+                               queryDataResponse: nil,
+                               expErrorSource:    ErrorSourcePlugin,
+                       },
                } {
                        t.Run(tc.name, func(t *testing.T) {
                                var actualCtx context.Context

--- FAIL: TestQueryData/TestQueryDataResponse/nil_queryDataResponse (0.00s)

--- FAIL: TestQueryData/TestQueryDataResponse (0.00s)

--- FAIL: TestQueryData (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xe6a4c5]

goroutine 73 [running]:
testing.tRunner.func1.2({0xf8e600, 0x1aa5d60})
	/usr/local/go/src/testing/testing.go:1632 +0x230
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1635 +0x35e
panic({0xf8e600?, 0x1aa5d60?})
	/usr/local/go/src/runtime/panic.go:785 +0x132
github.com/grafana/grafana-plugin-sdk-go/backend.ConvertToProtobuf.QueryDataResponse({}, 0x0)
	/home/giuseppe/grafana/grafana-plugin-sdk-go/backend/convert_to_protobuf.go:197 +0x25
github.com/grafana/grafana-plugin-sdk-go/backend.(*dataSDKAdapter).QueryData(0xc000474620, {0x1298120?, 0x1af9460?}, 0xc00011c700)
	/home/giuseppe/grafana/grafana-plugin-sdk-go/backend/data_adapter.go:109 +0x16e
github.com/grafana/grafana-plugin-sdk-go/backend.TestQueryData.func4.1(0xc000190820)
	/home/giuseppe/grafana/grafana-plugin-sdk-go/backend/data_adapter_test.go:195 +0x12a
testing.tRunner(0xc000190820, 0xc0004742d0)
	/usr/local/go/src/testing/testing.go:1690 +0xf4
created by testing.(*T).Run in goroutine 67
	/usr/local/go/src/testing/testing.go:1743 +0x390


Process finished with the exit code 1

So this section needs better error handling in general :)

[marefr]

Are you returning both a nil response and a nil error in your datasource?

[marefr]

I can see the test doing that, but was referring to your datasource @ivanahuckova

[ivanahuckova]

Are you returning both a nil response and a nil error in your datasource?

No, I was returning nil response and error. You can see it in the screenshot as well.

[marefr]

Pushed some changes:

• if error is not nil returns early
• If both error and resp is nil an error will be returned

[marefr]

This one I copied from

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

Lines 218 to 220 in e1e82c0

	if !status.IsValid() {
	status = statusFromError(dr.Error)
	}

[ivanahuckova]

Looks good! Thank you for tackling this. Left 1 comment bellow about panic caused by nil response. Otherwise LGTM.

[ivanahuckova]

It looks like that check if resp == nil || len(resp.Responses) == 0 { was needed also for code bellow, because now, if we return nil response, we get panic in for refID, r := range resp.Responses code

[ivanahuckova]

LGTM from me! 👏

[wbrowne]

LGTM