Skip to content

Commit a5274c4

Browse files
coryancoryan
committed
feat(storage): add debugging headers to ObjectWriteStream
Make it easier to troubleshoot problems with resumable uploads (aka `Client::WriteObject()`) by capturing the headers of the last request. We do not want to capture the headers for all the requests because there can be an unbounded number of requests, and therefore all the headers can consume arbitrary amounts of memory.
1 parent c58299d commit a5274c4

13 files changed

Lines changed: 50 additions & 17 deletions

‎google/cloud/storage/examples/storage_client_mock_samples.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,9 @@ TEST(StorageMockingSamples, MockWriteObject) {
8282
EXPECT_CALL(*mock, CreateResumableUpload)
8383
.WillOnce(Return(CreateResumableUploadResponse{"test-only-upload-id"}));
8484
EXPECT_CALL(*mock, UploadChunk)
85-
.WillOnce(Return(
86-
QueryResumableUploadResponse{/*committed_size=*/absl::nullopt,
87-
/*object_metadata=*/expected_metadata}));
85+
.WillOnce(Return(QueryResumableUploadResponse{
86+
/*.committed_size=*/absl::nullopt,
87+
/*.object_metadata=*/expected_metadata}));
8888

8989
auto stream = client.WriteObject("mock-bucket-name", "mock-object-name");
9090
stream << "Hello World!";

‎google/cloud/storage/internal/curl_request.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ StatusOr<HttpResponse> CurlRequest::MakeRequestImpl() {
132132
if (logging_enabled_) handle_.FlushDebug(__func__);
133133
auto code = handle_.GetResponseCode();
134134
if (!code.ok()) return std::move(code).status();
135+
received_headers_.emplace(":curl-peer", handle_.GetPeer());
135136
return HttpResponse{code.value(), std::move(response_payload_),
136137
std::move(received_headers_)};
137138
}

‎google/cloud/storage/internal/grpc_client.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,8 @@ StatusOr<QueryResumableUploadResponse> CloseWriteObjectStream(
149149
watchdog.cancel();
150150
if (watchdog.get()) return TimeoutError(timeout, "Close()");
151151
if (!response) return std::move(response).status();
152-
return GrpcObjectRequestParser::FromProto(*std::move(response), options);
152+
return GrpcObjectRequestParser::FromProto(*std::move(response), options,
153+
writer->GetRequestMetadata());
153154
}
154155

155156
} // namespace

‎google/cloud/storage/internal/grpc_object_request_parser.cc‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,8 @@ GrpcObjectRequestParser::ToProto(InsertObjectMediaRequest const& request) {
464464
}
465465

466466
QueryResumableUploadResponse GrpcObjectRequestParser::FromProto(
467-
google::storage::v2::WriteObjectResponse const& p, Options const& options) {
467+
google::storage::v2::WriteObjectResponse const& p, Options const& options,
468+
google::cloud::internal::StreamingRpcMetadata metadata) {
468469
QueryResumableUploadResponse response;
469470
if (p.has_persisted_size()) {
470471
response.committed_size = static_cast<std::uint64_t>(p.persisted_size());
@@ -473,6 +474,7 @@ QueryResumableUploadResponse GrpcObjectRequestParser::FromProto(
473474
response.payload =
474475
GrpcObjectMetadataParser::FromProto(p.resource(), options);
475476
}
477+
response.request_metadata = std::move(metadata);
476478
return response;
477479
}
478480

‎google/cloud/storage/internal/grpc_object_request_parser.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
#include "google/cloud/storage/internal/raw_client.h"
1919
#include "google/cloud/storage/version.h"
20+
#include "google/cloud/internal/grpc_request_metadata.h"
2021
#include <google/storage/v2/storage.pb.h>
2122

2223
namespace google {
@@ -47,8 +48,8 @@ struct GrpcObjectRequestParser {
4748
static StatusOr<google::storage::v2::WriteObjectRequest> ToProto(
4849
InsertObjectMediaRequest const& request);
4950
static QueryResumableUploadResponse FromProto(
50-
google::storage::v2::WriteObjectResponse const& p,
51-
Options const& options);
51+
google::storage::v2::WriteObjectResponse const& p, Options const& options,
52+
google::cloud::internal::StreamingRpcMetadata metadata);
5253

5354
static google::storage::v2::ListObjectsRequest ToProto(
5455
ListObjectsRequest const& request);

‎google/cloud/storage/internal/grpc_object_request_parser_test.cc‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ using ::google::cloud::testing_util::IsProtoEqual;
3434
using ::google::cloud::testing_util::StatusIs;
3535
using ::google::protobuf::TextFormat;
3636
using ::testing::ElementsAre;
37+
using ::testing::Pair;
3738
using ::testing::UnorderedElementsAre;
3839

3940
// Use gsutil to obtain the CRC32C checksum (in base64):
@@ -637,7 +638,7 @@ TEST(GrpcObjectRequestParser, WriteObjectResponseSimple) {
637638
)pb",
638639
&input));
639640

640-
auto const actual = GrpcObjectRequestParser::FromProto(input, Options{});
641+
auto const actual = GrpcObjectRequestParser::FromProto(input, Options{}, {});
641642
EXPECT_EQ(actual.committed_size.value_or(0), 123456);
642643
EXPECT_FALSE(actual.payload.has_value());
643644
}
@@ -653,12 +654,16 @@ TEST(GrpcObjectRequestParser, WriteObjectResponseWithResource) {
653654
})pb",
654655
&input));
655656

656-
auto const actual = GrpcObjectRequestParser::FromProto(input, Options{});
657+
auto const actual = GrpcObjectRequestParser::FromProto(
658+
input, Options{}, {{"header", "value"}, {"other-header", "other-value"}});
657659
EXPECT_FALSE(actual.committed_size.has_value());
658660
ASSERT_TRUE(actual.payload.has_value());
659661
EXPECT_EQ(actual.payload->name(), "test-object-name");
660662
EXPECT_EQ(actual.payload->bucket(), "test-bucket-name");
661663
EXPECT_EQ(actual.payload->size(), 123456);
664+
EXPECT_THAT(actual.request_metadata,
665+
UnorderedElementsAre(Pair("header", "value"),
666+
Pair("other-header", "other-value")));
662667
}
663668

664669
TEST(GrpcObjectRequestParser, ListObjectsRequestAllFields) {

‎google/cloud/storage/internal/object_requests.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,7 @@ StatusOr<std::uint64_t> ParseRangeHeader(std::string const& range) {
505505
StatusOr<QueryResumableUploadResponse>
506506
QueryResumableUploadResponse::FromHttpResponse(HttpResponse response) {
507507
QueryResumableUploadResponse result;
508+
result.request_metadata = std::move(response.headers);
508509
auto done = response.status_code == HttpStatusCode::kOk ||
509510
response.status_code == HttpStatusCode::kCreated;
510511

@@ -515,8 +516,8 @@ QueryResumableUploadResponse::FromHttpResponse(HttpResponse response) {
515516
if (!contents) return std::move(contents).status();
516517
result.payload = *std::move(contents);
517518
}
518-
auto r = response.headers.find("range");
519-
if (r == response.headers.end()) return result;
519+
auto r = result.request_metadata.find("range");
520+
if (r == result.request_metadata.end()) return result;
520521

521522
auto last_committed_byte = ParseRangeHeader(r->second);
522523
if (!last_committed_byte) return std::move(last_committed_byte).status();

‎google/cloud/storage/internal/object_requests.h‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "google/cloud/storage/well_known_parameters.h"
2929
#include "absl/types/optional.h"
3030
#include "absl/types/span.h"
31+
#include <map>
3132
#include <numeric>
3233
#include <string>
3334
#include <vector>
@@ -501,9 +502,22 @@ StatusOr<std::uint64_t> ParseRangeHeader(std::string const& range);
501502
struct QueryResumableUploadResponse {
502503
static StatusOr<QueryResumableUploadResponse> FromHttpResponse(
503504
HttpResponse response);
505+
QueryResumableUploadResponse() = default;
506+
QueryResumableUploadResponse(
507+
absl::optional<std::uint64_t> cs,
508+
absl::optional<google::cloud::storage::ObjectMetadata> p)
509+
: committed_size(std::move(cs)), payload(std::move(p)) {}
510+
QueryResumableUploadResponse(
511+
absl::optional<std::uint64_t> cs,
512+
absl::optional<google::cloud::storage::ObjectMetadata> p,
513+
std::multimap<std::string, std::string> rm)
514+
: committed_size(std::move(cs)),
515+
payload(std::move(p)),
516+
request_metadata(std::move(rm)) {}
504517

505518
absl::optional<std::uint64_t> committed_size;
506519
absl::optional<google::cloud::storage::ObjectMetadata> payload;
520+
std::multimap<std::string, std::string> request_metadata;
507521
};
508522

509523
bool operator==(QueryResumableUploadResponse const& lhs,

‎google/cloud/storage/internal/object_requests_test.cc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ using ::google::cloud::testing_util::StatusIs;
3232
using ::testing::ElementsAre;
3333
using ::testing::HasSubstr;
3434
using ::testing::Not;
35+
using ::testing::Pair;
36+
using ::testing::UnorderedElementsAre;
3537

3638
TEST(ObjectRequestsTest, ParseFailure) {
3739
auto actual = internal::ObjectMetadataParser::FromString("{123");
@@ -1006,6 +1008,10 @@ TEST(QueryResumableUploadResponseTest, Base) {
10061008
ASSERT_TRUE(actual.payload.has_value());
10071009
EXPECT_EQ("test-object-name", actual.payload->name());
10081010
EXPECT_EQ(2000, actual.committed_size.value_or(0));
1011+
EXPECT_THAT(actual.request_metadata,
1012+
UnorderedElementsAre(Pair("ignored-header", "value"),
1013+
Pair("location", "location-value"),
1014+
Pair("range", "bytes=0-1999")));
10091015

10101016
std::ostringstream os;
10111017
os << actual;

‎google/cloud/storage/internal/object_write_streambuf.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ void ObjectWriteStreambuf::AutoFlushFinal() {
6969
StatusOr<QueryResumableUploadResponse> ObjectWriteStreambuf::Close() {
7070
FlushFinal();
7171
if (!last_status_.ok()) return last_status_;
72-
return QueryResumableUploadResponse{committed_size_, metadata_};
72+
return QueryResumableUploadResponse{committed_size_, metadata_, headers_};
7373
}
7474

7575
bool ObjectWriteStreambuf::IsOpen() const {
@@ -159,6 +159,7 @@ void ObjectWriteStreambuf::FlushFinal() {
159159
} else {
160160
committed_size_ = response->committed_size.value_or(0);
161161
metadata_ = std::move(response->payload);
162+
headers_ = std::move(response->request_metadata);
162163
}
163164

164165
// Reset the iostream put area with valid pointers, but empty.

0 commit comments

Comments
 (0)