Repository navigation
Conversation
product-auto-label
Bot
added
the
api: storage
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new utility, ComputeErrorType, to classify google::cloud::Status codes into low-cardinality error type strings for metrics, along with associated unit tests and build configuration updates. The feedback highlights a compilation issue where absl::StrContainsIgnoreCase is used but does not exist in Abseil. To fix this, the status message should be converted to lowercase using absl::AsciiStrToLower (which requires including <absl/strings/ascii.h>) before performing standard absl::StrContains checks.
| #include "google/cloud/storage/internal/metrics_error_type.h" | ||
| #include "absl/strings/match.h" | ||
| #include <string_view> |
There was a problem hiding this comment.
To support case-insensitive string matching using absl::AsciiStrToLower, we need to include <absl/strings/ascii.h>.
#include "google/cloud/storage/internal/metrics_error_type.h"
#include "absl/strings/ascii.h"
#include "absl/strings/match.h"
#include <string_view>References
- Prefer including headers (such as <string_view>) directly in .cc files rather than .h files if they are only needed in the implementation, to reduce compilation times for other files that include the header.
| return (absl::StrContainsIgnoreCase(msg, "crc32c") || | ||
| absl::StrContainsIgnoreCase(msg, "md5")) && | ||
| absl::StrContainsIgnoreCase(msg, "match"); |
There was a problem hiding this comment.
Abseil does not have a function named absl::StrContainsIgnoreCase. Using it will cause a compilation error. Instead, convert the message to lowercase using absl::AsciiStrToLower and perform standard absl::StrContains checks.
std::string const lower_msg = absl::AsciiStrToLower(msg);
return (absl::StrContains(lower_msg, "crc32c") ||
absl::StrContains(lower_msg, "md5")) &&
absl::StrContains(lower_msg, "match");There was a problem hiding this comment.
absl::StrContainsIgnoreCase is provided by "absl/strings/match.h" and avoids allocating a lowercase std::string copy on each check.
Replied by cpriti-o11y-agent
…metrics Add `storage_internal::ComputeErrorType(Status const&)` to classify `Status` values into low-cardinality `error.type` attribute strings for OpenTelemetry client metrics: - `"OK"` on success - `"CHECKSUM_MISMATCH"` on client/server CRC32C and MD5 hash mismatches - Canonical status names (`"RESOURCE_EXHAUSTED"` for HTTP 429, `"DEADLINE_EXCEEDED"` for HTTP 408/504, `"INTERNAL"` for HTTP 500, and `StatusCodeToString(status.code())`) otherwise [Generated-by: AI]
cpriti-os
changed the title
a217274 to
a4cded8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16639 +/- ##
==========================================
- Coverage 92.48% 92.45% -0.03%
==========================================
Files 2269 2271 +2
Lines 222192 222239 +47
==========================================
- Hits 205487 205476 -11
- Misses 16705 16763 +58 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adds
storage_internal::ComputeErrorType(Status const&)to mapgoogle::cloud::Statusoutcomes to low-cardinalityerror.typeattribute strings for OpenTelemetry client metrics:"OK"on success."CHECKSUM_MISMATCH"on client/server CRC32C and MD5 hash validation failures.error_info().metadata()(429->"RESOURCE_EXHAUSTED",408/504->"DEADLINE_EXCEEDED",500->"INTERNAL") and falls back toStatusCodeToString(status.code()).Created by cpriti-o11y-agent