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

blob: include key in error messages #2565

Merged
merged 4 commits into from Jul 16, 2019
Merged

blob: include key in error messages #2565

merged 4 commits into from Jul 16, 2019

Conversation

vangent
Copy link
Contributor

@vangent vangent commented Jul 16, 2019

Fixes #2564.

See the bug for context. TL;DR, services don't usually include the key in error messages (e.g., "The specified key does not exist" without saying what the key was). That lack of info can make debugging difficult. This PR adds the key (where possible) to error messages from blob, via the portable type.

For example, the GCS error for a missing blob was:

blob (code=NotFound): storage: object doesn't exist

and after this change it is:

blob (key not-found) (code=NotFound): storage: object doesn't exist

I also added a conformance test for Bucket.Attributes for a nonexistent key.

@vangent vangent requested a review from shantuo July 16, 2019 16:09
@googlebot googlebot added the cla: yes Google CLA has been signed! label Jul 16, 2019
@@ -768,8 +772,6 @@ func (b *Bucket) NewWriter(ctx context.Context, key string, opts *WriterOptions)
end: end,
cancel: cancel,
key: key,
opts: dopts,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two fields (along with ctx) are only supposed to be set when we need to call NewTypedWriter later; they are explicitly set in the else a few lines down. I'm not sure why we were setting them here, they are not needed and it made the code unclear because the comment in the struct says they are only set when w is nil.

I removed them and clarified the comments.

@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #2565 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2565      +/-   ##
==========================================
+ Coverage   66.65%   66.71%   +0.06%     
==========================================
  Files         133      133              
  Lines       15489    15488       -1     
==========================================
+ Hits        10324    10333       +9     
+ Misses       4398     4387      -11     
- Partials      767      768       +1
Impacted Files Coverage Δ
blob/blob.go 92.53% <100%> (-0.02%) ⬇️
internal/cmd/gocdk/main.go 84.84% <0%> (-2.03%) ⬇️
docstore/docstore.go 83.39% <0%> (-1.89%) ⬇️
pubsub/rabbitpubsub/rabbit.go 80.45% <0%> (-0.98%) ⬇️
runtimevar/runtimevar.go 96% <0%> (ø) ⬆️
pubsub/pubsub.go 91.84% <0%> (ø) ⬆️
blob/azureblob/azureblob.go 88.5% <0%> (+0.5%) ⬆️
blob/fileblob/fileblob.go 79.41% <0%> (+0.58%) ⬆️
blob/gcsblob/gcsblob.go 90.34% <0%> (+0.68%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be32565...e289403. Read the comment docs.

Copy link
Contributor

@shantuo shantuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one nit.

blob/blob.go Outdated Show resolved Hide resolved
@vangent vangent merged commit 298c49e into google:master Jul 16, 2019
@vangent vangent deleted the blob-err branch July 16, 2019 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA has been signed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

blob: include blob key in errors
3 participants