From e18d029a241b2515fdeab8e6c61a8608d474663b Mon Sep 17 00:00:00 2001 From: Andreas Christou Date: Fri, 20 Sep 2024 10:40:05 +0100 Subject: [PATCH] GCM: Improve error handling (#93474) * Improve error handling * More manual errorsource * Revert some changes from original PR * Update test --- pkg/tsdb/cloud-monitoring/annotation_query.go | 13 +++++++++--- pkg/tsdb/cloud-monitoring/cloudmonitoring.go | 21 ++++++++++--------- pkg/tsdb/cloud-monitoring/promql_query.go | 8 ++++--- .../cloud-monitoring/promql_query_test.go | 6 ++++-- pkg/tsdb/cloud-monitoring/resource_handler.go | 2 +- .../cloud-monitoring/time_series_filter.go | 5 +++-- .../cloud-monitoring/time_series_query.go | 3 ++- pkg/tsdb/cloud-monitoring/utils.go | 11 ++++++---- 8 files changed, 43 insertions(+), 26 deletions(-) diff --git a/pkg/tsdb/cloud-monitoring/annotation_query.go b/pkg/tsdb/cloud-monitoring/annotation_query.go index 175919067312..5db953fef29f 100644 --- a/pkg/tsdb/cloud-monitoring/annotation_query.go +++ b/pkg/tsdb/cloud-monitoring/annotation_query.go @@ -23,7 +23,10 @@ type annotationEvent struct { func (s *Service) executeAnnotationQuery(ctx context.Context, req *backend.QueryDataRequest, dsInfo datasourceInfo, queries []cloudMonitoringQueryExecutor, logger log.Logger) ( *backend.QueryDataResponse, error) { resp := backend.NewQueryDataResponse() - queryRes, dr, _, err := queries[0].run(ctx, req, s, dsInfo, logger) + dr, queryRes, _, err := queries[0].run(ctx, req, s, dsInfo, logger) + if dr.Error != nil { + errorsource.AddErrorToResponse(queries[0].getRefID(), resp, dr.Error) + } if err != nil { errorsource.AddErrorToResponse(queries[0].getRefID(), resp, err) return resp, err @@ -39,10 +42,14 @@ func (s *Service) executeAnnotationQuery(ctx context.Context, req *backend.Query firstQuery := req.Queries[0] err = json.Unmarshal(firstQuery.JSON, &tslq) if err != nil { + logger.Error("error unmarshaling query", "error", err, "statusSource", backend.ErrorSourceDownstream) + errorsource.AddErrorToResponse(firstQuery.RefID, resp, err) return resp, nil } - err = parseToAnnotations(req.Queries[0].RefID, queryRes, dr.(cloudMonitoringResponse), tslq.TimeSeriesList.Title, tslq.TimeSeriesList.Text) - resp.Responses[firstQuery.RefID] = *queryRes + + // parseToAnnotations never actually returns an error + err = parseToAnnotations(req.Queries[0].RefID, dr, queryRes.(cloudMonitoringResponse), tslq.TimeSeriesList.Title, tslq.TimeSeriesList.Text) + resp.Responses[firstQuery.RefID] = *dr if err != nil { errorsource.AddErrorToResponse(firstQuery.RefID, resp, err) diff --git a/pkg/tsdb/cloud-monitoring/cloudmonitoring.go b/pkg/tsdb/cloud-monitoring/cloudmonitoring.go index 1ff0fcf9747a..c8fd4c85147b 100644 --- a/pkg/tsdb/cloud-monitoring/cloudmonitoring.go +++ b/pkg/tsdb/cloud-monitoring/cloudmonitoring.go @@ -361,20 +361,21 @@ func (s *Service) executeTimeSeriesQuery(ctx context.Context, req *backend.Query *backend.QueryDataResponse, error) { resp := backend.NewQueryDataResponse() for _, queryExecutor := range queries { - queryRes, dr, executedQueryString, err := queryExecutor.run(ctx, req, s, dsInfo, logger) + dr, queryRes, executedQueryString, err := queryExecutor.run(ctx, req, s, dsInfo, logger) if err != nil { errorsource.AddErrorToResponse(queryExecutor.getRefID(), resp, err) return resp, err } - err = queryExecutor.parseResponse(queryRes, dr, executedQueryString, logger) + err = queryExecutor.parseResponse(dr, queryRes, executedQueryString, logger) if err != nil { - // Default to a plugin error if there's no source + dr.Error = err + // // Default to a plugin error if there's no source errWithSource := errorsource.SourceError(backend.ErrorSourcePlugin, err, false) - queryRes.Error = errWithSource.Unwrap() - queryRes.ErrorSource = errWithSource.ErrorSource() + dr.Error = errWithSource.Unwrap() + dr.ErrorSource = errWithSource.ErrorSource() } - resp.Responses[queryExecutor.getRefID()] = *queryRes + resp.Responses[queryExecutor.getRefID()] = *dr } return resp, nil @@ -608,21 +609,21 @@ func unmarshalResponse(res *http.Response, logger log.Logger) (cloudMonitoringRe }() if res.StatusCode/100 != 2 { - logger.Error("Request failed", "status", res.Status, "body", string(body)) + logger.Error("Request failed", "status", res.Status, "body", string(body), "statusSource", backend.ErrorSourceDownstream) return cloudMonitoringResponse{}, errorsource.SourceError(backend.ErrorSourceFromHTTPStatus(res.StatusCode), fmt.Errorf("query failed: %s", string(body)), false) } var data cloudMonitoringResponse err = json.Unmarshal(body, &data) if err != nil { - logger.Error("Failed to unmarshal CloudMonitoring response", "error", err, "status", res.Status, "body", string(body)) + logger.Error("Failed to unmarshal CloudMonitoring response", "error", err, "status", res.Status, "body", string(body), "statusSource", backend.ErrorSourceDownstream) return cloudMonitoringResponse{}, fmt.Errorf("failed to unmarshal query response: %w", err) } return data, nil } -func addConfigData(frames data.Frames, dl string, unit string, period *string) data.Frames { +func addConfigData(frames data.Frames, dl string, unit string, period *string, logger log.Logger) data.Frames { for i := range frames { if frames[i].Fields[1].Config == nil { frames[i].Fields[1].Config = &data.FieldConfig{} @@ -646,7 +647,7 @@ func addConfigData(frames data.Frames, dl string, unit string, period *string) d if period != nil && *period != "" { err := addInterval(*period, frames[i].Fields[0]) if err != nil { - backend.Logger.Error("Failed to add interval", "error", err) + logger.Error("Failed to add interval: %s", err, "statusSource", backend.ErrorSourceDownstream) } } } diff --git a/pkg/tsdb/cloud-monitoring/promql_query.go b/pkg/tsdb/cloud-monitoring/promql_query.go index 1dc6923bb0db..b6d14c7c253a 100644 --- a/pkg/tsdb/cloud-monitoring/promql_query.go +++ b/pkg/tsdb/cloud-monitoring/promql_query.go @@ -23,7 +23,8 @@ func (promQLQ *cloudMonitoringProm) run(ctx context.Context, req *backend.QueryD dr := &backend.DataResponse{} projectName, err := s.ensureProject(ctx, dsInfo, promQLQ.parameters.ProjectName) if err != nil { - return dr, backend.DataResponse{}, "", err + dr.Error = err + return dr, backend.DataResponse{}, "", nil } r, err := createRequest(ctx, &dsInfo, path.Join("/v1/projects", projectName, "location/global/prometheus/api/v1/query_range"), nil) if err != nil { @@ -43,12 +44,13 @@ func (promQLQ *cloudMonitoringProm) run(ctx context.Context, req *backend.QueryD res, err := doRequestProm(r, dsInfo, requestBody) if err != nil { - return dr, backend.DataResponse{}, "", err + dr.Error = err + return dr, backend.DataResponse{}, "", nil } defer func() { if err := res.Body.Close(); err != nil { - s.logger.Error("Failed to close response body", "err", err) + s.logger.Error("Failed to close response body", "err", err, "statusSource", backend.ErrorSourceDownstream) } }() diff --git a/pkg/tsdb/cloud-monitoring/promql_query_test.go b/pkg/tsdb/cloud-monitoring/promql_query_test.go index a21d6dce7938..2d8421e1e54a 100644 --- a/pkg/tsdb/cloud-monitoring/promql_query_test.go +++ b/pkg/tsdb/cloud-monitoring/promql_query_test.go @@ -59,8 +59,10 @@ func TestPromqlQuery(t *testing.T) { } dr, parsedProm, _, err := query.run(context.Background(), &backend.QueryDataRequest{}, service, dsInfo, service.logger) - require.Error(t, err) - require.Equal(t, "not found!", err.Error()) + require.NoError(t, err) + require.Error(t, dr.Error) + require.Equal(t, "not found!", dr.Error.Error()) + require.True(t, backend.IsDownstreamError(dr.Error)) err = query.parseResponse(dr, parsedProm, "", service.logger) require.NoError(t, err) diff --git a/pkg/tsdb/cloud-monitoring/resource_handler.go b/pkg/tsdb/cloud-monitoring/resource_handler.go index afcce954784a..c9b23b036a42 100644 --- a/pkg/tsdb/cloud-monitoring/resource_handler.go +++ b/pkg/tsdb/cloud-monitoring/resource_handler.go @@ -386,7 +386,7 @@ func writeResponseBytes(rw http.ResponseWriter, code int, msg []byte) { rw.WriteHeader(code) _, err := rw.Write(msg) if err != nil { - backend.Logger.Error("Unable to write HTTP response", "error", err) + backend.Logger.Error("Unable to write HTTP response", "error", err, "statusSource", backend.ErrorSourceDownstream) } } diff --git a/pkg/tsdb/cloud-monitoring/time_series_filter.go b/pkg/tsdb/cloud-monitoring/time_series_filter.go index 5a387f8990d2..5928840f5997 100644 --- a/pkg/tsdb/cloud-monitoring/time_series_filter.go +++ b/pkg/tsdb/cloud-monitoring/time_series_filter.go @@ -21,7 +21,7 @@ func (timeSeriesFilter *cloudMonitoringTimeSeriesList) run(ctx context.Context, } func parseTimeSeriesResponse(queryRes *backend.DataResponse, - response cloudMonitoringResponse, executedQueryString string, query cloudMonitoringQueryExecutor, params url.Values, groupBys []string, _ log.Logger) error { + response cloudMonitoringResponse, executedQueryString string, query cloudMonitoringQueryExecutor, params url.Values, groupBys []string, logger log.Logger) error { frames := data.Frames{} for _, series := range response.TimeSeries { @@ -47,7 +47,7 @@ func parseTimeSeriesResponse(queryRes *backend.DataResponse, if len(response.TimeSeries) > 0 { dl := query.buildDeepLink() aggregationAlignmentString := params.Get("aggregation.alignmentPeriod") - frames = addConfigData(frames, dl, response.Unit, &aggregationAlignmentString) + frames = addConfigData(frames, dl, response.Unit, &aggregationAlignmentString, logger) } queryRes.Frames = frames @@ -93,6 +93,7 @@ func (timeSeriesFilter *cloudMonitoringTimeSeriesList) buildDeepLink() string { "Failed to generate deep link: unable to parse metrics explorer URL", "ProjectName", timeSeriesFilter.parameters.ProjectName, "error", err, + "statusSource", backend.ErrorSourcePlugin, ) } diff --git a/pkg/tsdb/cloud-monitoring/time_series_query.go b/pkg/tsdb/cloud-monitoring/time_series_query.go index 4a7f29f72069..13bd03d7d6e7 100644 --- a/pkg/tsdb/cloud-monitoring/time_series_query.go +++ b/pkg/tsdb/cloud-monitoring/time_series_query.go @@ -78,7 +78,7 @@ func (timeSeriesQuery *cloudMonitoringTimeSeriesQuery) parseResponse(queryRes *b } if len(response.TimeSeriesData) > 0 { dl := timeSeriesQuery.buildDeepLink() - frames = addConfigData(frames, dl, response.Unit, timeSeriesQuery.parameters.GraphPeriod) + frames = addConfigData(frames, dl, response.Unit, timeSeriesQuery.parameters.GraphPeriod, logger) } queryRes.Frames = frames @@ -106,6 +106,7 @@ func (timeSeriesQuery *cloudMonitoringTimeSeriesQuery) buildDeepLink() string { "Failed to generate deep link: unable to parse metrics explorer URL", "ProjectName", timeSeriesQuery.parameters.Query, "error", err, + "statusSource", backend.ErrorSourcePlugin, ) } diff --git a/pkg/tsdb/cloud-monitoring/utils.go b/pkg/tsdb/cloud-monitoring/utils.go index f9d108082288..e95e66c681af 100644 --- a/pkg/tsdb/cloud-monitoring/utils.go +++ b/pkg/tsdb/cloud-monitoring/utils.go @@ -61,7 +61,7 @@ func createRequest(ctx context.Context, dsInfo *datasourceInfo, proxyPass string } req, err := http.NewRequestWithContext(ctx, method, dsInfo.services[cloudMonitor].url, body) if err != nil { - backend.Logger.Error("Failed to create request", "error", err) + backend.Logger.Error("Failed to create request", "error", err, "statusSource", backend.ErrorSourceDownstream) return nil, fmt.Errorf("failed to create request: %w", err) } @@ -142,7 +142,8 @@ func runTimeSeriesRequest(ctx context.Context, req *backend.QueryDataRequest, dr := &backend.DataResponse{} projectName, err := s.ensureProject(ctx, dsInfo, projectName) if err != nil { - return dr, cloudMonitoringResponse{}, "", err + dr.Error = err + return dr, cloudMonitoringResponse{}, "", nil } timeSeriesMethod := "timeSeries" if body != nil { @@ -150,7 +151,8 @@ func runTimeSeriesRequest(ctx context.Context, req *backend.QueryDataRequest, } r, err := createRequest(ctx, &dsInfo, path.Join("/v3/projects", projectName, timeSeriesMethod), nil) if err != nil { - return dr, cloudMonitoringResponse{}, "", err + dr.Error = err + return dr, cloudMonitoringResponse{}, "", nil } span := traceReq(ctx, req, dsInfo, r, params.Encode()) @@ -158,7 +160,8 @@ func runTimeSeriesRequest(ctx context.Context, req *backend.QueryDataRequest, d, err := doRequestWithPagination(ctx, r, dsInfo, params, body, logger) if err != nil { - return dr, cloudMonitoringResponse{}, "", err + dr.Error = err + return dr, cloudMonitoringResponse{}, "", nil } return dr, d, r.URL.RawQuery, nil