Skip to content
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

all: do not hide original error code #235

Open
kokizzu opened this issue Oct 18, 2022 · 4 comments
Open

all: do not hide original error code #235

kokizzu opened this issue Oct 18, 2022 · 4 comments
Assignees
Labels
external This issue is blocked on a bug with the actual product. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@kokizzu
Copy link

kokizzu commented Oct 18, 2022

Is your feature request related to a problem? Please describe.
this code hides original error message into context deadline exceeded
cloud.google.com/go/bigquery/storage/apiv1/big_query_write_client.go:396

func (c *bigQueryWriteGRPCClient) GetWriteStream(ctx context.Context, req *storagepb.GetWriteStreamRequest, opts ...gax.CallOption) (*storagepb.WriteStream, error) {
	if _, ok := ctx.Deadline(); !ok && !c.disableDeadlines {
		cctx, cancel := context.WithTimeout(ctx, 600000*time.Millisecond)
		defer cancel()
		ctx = cctx
	}
	md := metadata.Pairs("x-goog-request-params", fmt.Sprintf("%s=%v", "name", url.QueryEscape(req.GetName())))

	ctx = insertMetadata(ctx, c.xGoogMetadata, md)
	opts = append((*c.CallOptions).GetWriteStream[0:len((*c.CallOptions).GetWriteStream):len((*c.CallOptions).GetWriteStream)], opts...)
	var resp *storagepb.WriteStream
	err := gax.Invoke(ctx, func(ctx context.Context, settings gax.CallSettings) error {
		var err error
		resp, err = c.bigQueryWriteClient.GetWriteStream(ctx, req, settings.GRPC...)
		return err // ----> this one can be anything, eg. API limit or something
	}, opts...)
	if err != nil {
		return nil, err // ----> this one context deadline exceeded
	}
	return resp, nil
}

Describe the solution you'd like
it's better to return last error instead so we can know what's the cause of the context deadline exceeded

Describe alternatives you've considered
i don't see any other alternative

Additional context
annoying to trace to know what's the cause of context deadline exceded when the original error hidden.

@kokizzu kokizzu added the triage me I really want to be triaged. label Oct 18, 2022
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the BigQuery API. label Oct 18, 2022
@codyoss
Copy link
Member

codyoss commented Oct 18, 2022

This is raised as a bigquery issue, but I think that it is likely an issue in all of our gapics. We just fixed a similarish issue recently on the apiary space: googleapis/google-api-go-client#1684

I wonder if we should be doing something similar for our gapic clients. cc @noahdietz

@codyoss codyoss changed the title bigquery: do not hide original error code all: do not hide original error code Oct 18, 2022
@codyoss codyoss added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed api: bigquery Issues related to the BigQuery API. triage me I really want to be triaged. labels Oct 18, 2022
@codyoss codyoss assigned codyoss and unassigned shollyman Oct 18, 2022
@noahdietz
Copy link
Contributor

My read on the issue is that the RPC response was a status code configured for retries, gax slept to delay the next attempt and in doing so exceeded the context deadline (or the context was canceled while sleeping), resulting in a context.DeadlineExceeded being returned (from here) instead of the error response status code that caused it to attempt a retry in the first place.

That is how I see this being able to happen.

We likely need to do wrapping similar to the stuff @codyoss mentioned but in gax here.

@codyoss
Copy link
Member

codyoss commented Oct 18, 2022

I am going to transfer this issue over to gax-go, seems like a reasonable request to me.

@codyoss codyoss transferred this issue from googleapis/google-cloud-go Oct 18, 2022
@codyoss
Copy link
Member

codyoss commented May 16, 2023

I have dug into this deeper and I think for now we should put this on hold. Although it would be for the better of all, we can't make this change as a non-breaking change, and this is considered a stable library. The good news is that in a few months there will be a way to add a cause to deadline exceeded errors and I think we can provide more details that way without breaking anyone. This is a parallel feature to https://pkg.go.dev/context#WithCancelCause added in 1.20. Once 1.21 comes out we can considered adding this functionality with build tags.

Relevant prop: golang/go#56661

@codyoss codyoss added the external This issue is blocked on a bug with the actual product. label May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external This issue is blocked on a bug with the actual product. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

4 participants