cleanup(storage): make logs more readable (#9761)

Author: coryan

google/cloud/internal/binary_data_as_debug_string.cc

@@ -13,72 +13,24 @@
 // limitations under the License.
 
 #include "google/cloud/internal/binary_data_as_debug_string.h"
-#include <array>
+#include <algorithm>
 #include <cctype>
-#include <limits>
 
 namespace google {
 namespace cloud {
 namespace rest_internal {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
+
 std::string BinaryDataAsDebugString(char const* data, std::size_t size,
                                     std::size_t max_output_bytes) {
-  // We want about 2/3 of a standard 80 column terminal to be used by the hex
-  // representation and the other 1/3 (because it is half as wide) with the
-  // text representation. Setting this value to 24 uses 72 columns: 48 for the
-  // hex representation, 24 for text, and one space. We could use 25 or 26, we
-  // chose (somewhat arbitrarily) 24 as it is 16 + 8 and thus more "round" in
-  // base 2.
-  auto constexpr kTextWidth = 24;
-  std::string result;
-  std::string text_column(kTextWidth, ' ');
-  std::string hex_column(2 * kTextWidth, ' ');
-
-  // Capture everything (which we use anyway) because:
-  //   - clang-tidy complains if you capture a constexpr or const
-  //   - MSVC does not automatically capture constexpr types unless we do this
-  auto flush = [&] {
-    result += text_column;
-    result += ' ';
-    result += hex_column;
-    result += '\n';
-    text_column = std::string(kTextWidth, ' ');
-    hex_column = std::string(2 * kTextWidth, ' ');
-  };
-
-  // Limit the output to the first `max_output_bytes`.
-  std::size_t n = size;
-  if (max_output_bytes > 0 && max_output_bytes < size) {
-    n = max_output_bytes;
-  }
-
-  std::size_t count = 0;
-  for (char const* c = data; c != data + n; ++c) {
-    // std::isprint() actually takes an int argument, signed, without this
-    // explicit conversion MSVC in Debug mode asserts an invalid argument, and
-    // pops up a nice dialog box that breaks the CI builds.
-    int cval = static_cast<unsigned char>(*c);
-    if (std::isprint(cval) != 0) {
-      text_column[count] = *c;
-    } else {
-      text_column[count] = '.';
-    }
-    auto constexpr kCharHexWidth = 2;
-    std::array<char, kCharHexWidth + 1> buf{};
-    snprintf(buf.data(), buf.size(), "%02x", cval);
-    hex_column[2 * count] = buf[0];
-    hex_column[2 * count + 1] = buf[1];
-    ++count;
-    if (count == kTextWidth) {
-      flush();
-      count = 0;
-    }
-  }
-  if (count != 0) {
-    flush();
-  }
-  return result;
+  auto dump = std::string{
+      data, max_output_bytes == 0 ? size : std::min(size, max_output_bytes)};
+  std::transform(dump.begin(), dump.end(), dump.begin(),
+                 [](auto c) { return std::isprint(c) ? c : '.'; });
+  if (max_output_bytes == 0 || size <= max_output_bytes) return dump;
+  return dump + "...<truncated>...";
 }
+
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace rest_internal
 }  // namespace cloud

google/cloud/internal/binary_data_as_debug_string.h

@@ -22,17 +22,16 @@ namespace google {
 namespace cloud {
 namespace rest_internal {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
+
 /**
  * Formats a block of data for debug printing.
  *
- * Takes a block of data, possible with non-printable characters and creates
- * a string with two columns.  The first column is 24 characters wide and has
- * the non-printable characters replaced by periods.  The second column is 48
- * characters wide and contains the hexdump of the data.  The columns are
- * separated by a single space.
+ * Converts non-printable characters to '.'. If requested, it truncates the
+ * output and adds a '...<truncated>...' marker when it does so.
  */
 std::string BinaryDataAsDebugString(char const* data, std::size_t size,
                                     std::size_t max_output_bytes = 0);
+
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace rest_internal
 }  // namespace cloud

google/cloud/internal/binary_data_as_debug_string_test.cc

@@ -22,47 +22,20 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 namespace {
 
 TEST(BinaryDataAsDebugStringTest, Simple) {
-  auto actual = BinaryDataAsDebugString("123abc", 6);
-  EXPECT_EQ(
-      "123abc                   "
-      "313233616263                                    \n",
-      actual);
-}
-
-TEST(BinaryDataAsDebugStringTest, Multiline) {
-  auto actual =
-      BinaryDataAsDebugString(" 123456789 123456789 123456789 123456789", 40);
-  EXPECT_EQ(
-      " 123456789 123456789 123 "
-      "203132333435363738392031323334353637383920313233\n"
-      "456789 123456789         "
-      "34353637383920313233343536373839                \n",
-      actual);
-}
-
-TEST(BinaryDataAsDebugStringTest, Blanks) {
-  auto actual = BinaryDataAsDebugString("\n \r \t \v \b \a \f ", 14);
-  EXPECT_EQ(
-      ". . . . . . .            "
-      "0a200d2009200b20082007200c20                    \n",
-      actual);
-}
-
-TEST(BinaryDataAsDebugStringTest, NonPrintable) {
-  auto actual = BinaryDataAsDebugString("\x03\xf1 abcd", 7);
-  EXPECT_EQ(
-      ".. abcd                  "
-      "03f12061626364                                  \n",
-      actual);
-}
-
-TEST(BinaryDataAsDebugStringTest, Limit) {
-  auto actual = BinaryDataAsDebugString(
-      " 123456789 123456789 123456789 123456789", 40, 24);
-  EXPECT_EQ(
-      " 123456789 123456789 123 "
-      "203132333435363738392031323334353637383920313233\n",
-      actual);
+  struct TestCase {
+    std::string input;
+    std::size_t max;
+    std::string expected;
+  } cases[] = {
+      {"123abc", 0, "123abc"},
+      {"234abc", 3, "234...<truncated>..."},
+      {"3\n4\n5abc", 0, "3.4.5abc"},
+      {"3\n4\n5a\n\n\nbc", 5, "3.4.5...<truncated>..."},
+  };
+  for (auto const& t : cases) {
+    EXPECT_EQ(t.expected,
+              BinaryDataAsDebugString(t.input.data(), t.input.size(), t.max));
+  }
 }
 
 }  // namespace

google/cloud/storage/google_cloud_cpp_storage.bzl

@@ -148,7 +148,6 @@ google_cloud_cpp_storage_srcs = [
     "idempotency_policy.cc",
     "internal/access_control_common_parser.cc",
     "internal/access_token_credentials.cc",
-    "internal/binary_data_as_debug_string.cc",
     "internal/bucket_access_control_parser.cc",
     "internal/bucket_acl_requests.cc",
     "internal/bucket_metadata_parser.cc",

google/cloud/storage/google_cloud_cpp_storage.cmake

@@ -47,7 +47,6 @@ add_library(
     internal/access_control_common_parser.h
     internal/access_token_credentials.cc
     internal/access_token_credentials.h
-    internal/binary_data_as_debug_string.cc
     internal/binary_data_as_debug_string.h
     internal/bucket_access_control_parser.cc
     internal/bucket_access_control_parser.h
@@ -437,7 +436,6 @@ if (BUILD_TESTING)
         internal/access_control_common_parser_test.cc
         internal/access_control_common_test.cc
         internal/access_token_credentials_test.cc
-        internal/binary_data_as_debug_string_test.cc
         internal/bucket_acl_requests_test.cc
         internal/bucket_requests_test.cc
         internal/complex_option_test.cc

google/cloud/storage/internal/binary_data_as_debug_string.cc

@@ -16,24 +16,16 @@
 #define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_STORAGE_INTERNAL_BINARY_DATA_AS_DEBUG_STRING_H
 
 #include "google/cloud/storage/version.h"
-#include <string>
+#include "google/cloud/internal/binary_data_as_debug_string.h"
 
 namespace google {
 namespace cloud {
 namespace storage {
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN
 namespace internal {
-/**
- * Formats a block of data for debug printing.
- *
- * Takes a block of data, possible with non-printable characters and creates
- * a string with two columns.  The first column is 24 characters wide and has
- * the non-printable characters replaced by periods.  The second column is 48
- * characters wide and contains the hexdump of the data.  The columns are
- * separated by a single space.
- */
-std::string BinaryDataAsDebugString(char const* data, std::size_t size,
-                                    std::size_t max_output_bytes = 0);
+
+using ::google::cloud::rest_internal::BinaryDataAsDebugString;
+
 }  // namespace internal
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace storage