Hi @ddelgrosso1 , Please see this approach here for creating the traces of UploadFile method, if this looks good, then i will go ahead and update the test files.

@shubham-up-47 I think overall the approach makes sense to me. You might want to run this by @scotthart as well to get a second, more informed opinion.

Codecov Report

Attention: Patch coverage is 81.81818% with 58 lines in your changes missing coverage. Please review.

Project coverage is 92.92%. Comparing base (0819ec0) to head (894e71b).
Report is 5 commits behind head on main.

@@            Coverage Diff             @@
##             main   #15245      +/-   ##
==========================================
- Coverage   92.93%   92.92%   -0.01%     
==========================================
  Files        2394     2396       +2     
  Lines      215564   215816     +252     
==========================================
+ Hits       200332   200553     +221     
- Misses      15232    15263      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions

google/cloud/storage/internal/connection_impl.cc line 768 at r1 (raw file):

    os << __func__ << "(" << request << ", " << file_name
       << "): UploadFromOffset (" << upload_offset
       << ") is bigger than the size of file source (" << file_size << ")";

Let's update this and other usages of std::ostringstream being relocated in this PR to use absl::StrCat in this internal file.

Code quote:

    std::ostringstream os;
    os << __func__ << "(" << request << ", " << file_name
       << "): UploadFromOffset (" << upload_offset
       << ") is bigger than the size of file source (" << file_size << ")";

google/cloud/storage/client.h line 3547 at r1 (raw file):

  StatusOr<ObjectMetadata> UploadFileResumable(
      std::string const& file_name, internal::ResumableUploadRequest request);

The purpose of these helper functions is to move the code that doesn't rely on the templated types in UploadFileImpl in to the .cc file. Let's keep these functions but re-purpose them to contain the code added to UploadFileImpl.

Code quote:

  StatusOr<ObjectMetadata> UploadFileSimple(
      std::string const& file_name, std::size_t file_size,
      internal::InsertObjectMediaRequest request);

  StatusOr<ObjectMetadata> UploadFileResumable(
      std::string const& file_name, internal::ResumableUploadRequest request);

google/cloud/storage/internal/storage_connection.h line 118 at r1 (raw file):

  virtual StatusOr<std::unique_ptr<std::istream>> UploadFileResumable(
      std::string const& file_name,
      ResumableUploadRequest& request) = 0;

I'm concerned that adding new pure abstract methods to this class could break customers using a Mock or other derivation of this class. Instead, let's make these methods have a default implementation that returns a kUNIMPLEMENTED Status.

Code quote:

  virtual StatusOr<EmptyResponse> UploadFileSimple(
      std::string const& file_name, std::size_t file_size,
      InsertObjectMediaRequest& request) = 0;
  virtual StatusOr<std::unique_ptr<std::istream>> UploadFileResumable(
      std::string const& file_name,
      ResumableUploadRequest& request) = 0;

1. Getting error while using absl::Strcat, since absl::StrCat can’t convert request to string format.
2. Done, Keeping these functions and re-purposed them to contain the code added.
3. Done, Keeping a default implementation which returns a kUNIMPLEMENTED status.

Reviewed 4 of 11 files at r2, 6 of 11 files at r3, all commit messages.
Reviewable status: 10 of 15 files reviewed, 5 unresolved discussions (waiting on @shubham-up-47)

google/cloud/storage/internal/storage_connection.h line 118 at r3 (raw file):

      [[maybe_unused]] InsertObjectMediaRequest& request) {
    return Status(StatusCode::kUnimplemented, "unimplemented");
  }

There's not need to use maybe_unused here, simply omit the names of the parameters:

virtual StatusOrstd::unique_ptrstd::string UploadFileSimple( std::string const&, std::size_t, InsertObjectMediaRequest&) { return Status(StatusCode::kUnimplemented, "unimplemented"); }

Code quote:

  virtual StatusOr<std::unique_ptr<std::string>> UploadFileSimple(
      [[maybe_unused]] std::string const& file_name,
      [[maybe_unused]] std::size_t file_size,
      [[maybe_unused]] InsertObjectMediaRequest& request) {
    return Status(StatusCode::kUnimplemented, "unimplemented");
  }

google/cloud/storage/client_object_test.cc line 263 at r3 (raw file):

  auto expected =
      storage::internal::ObjectMetadataParser::FromString(text).value();
  auto const contents = std::string{"some simple contents"};

When declaring simple variables like this, prefer not using auto, instead:

std::string const contents{"some simple contents"};

Here and elsewhere in this PR.

1. Done, was using may_be to pass code coverage, didn't know this solution, thanks for telling it.
2. Done, will keep that thing in mind and avoid auto for simple variables.

Reviewed 2 of 11 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ddelgrosso1 and @shubham-up-47)

google/cloud/storage/internal/storage_connection.h line 118 at r1 (raw file):

Previously, shubham-up-47 wrote…
re: ostringstream and absl::StrCat, let's not block this PR on that. Please create an internal cleanup issue to convert the ostringstream uses in storage internal to use absl::StrCat, we can address it in the future.

Done, created it #15267