feat: expose and log ErrorInfo if present #7640
Conversation
google-cla
bot
added
the
cla: yes
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7640 +/- ##
==========================================
- Coverage 95.28% 95.28% -0.01%
==========================================
Files 1254 1254
Lines 113126 113226 +100
==========================================
+ Hits 107793 107885 +92
- Misses 5333 5341 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
A few last-minute nits, sorry about that. I think we should keep #7429 open until the storage library also supports ErrorInfo? We can fix that in a separate PR, of course.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @devjgm)
google/cloud/grpc_error_delegate.cc, line 73 at r1 (raw file):
google::rpc::Status const& proto) { for (google::protobuf::Any const& any : proto.details()) { if (any.Is<google::rpc::ErrorInfo>()) {
nit: what happens if there is more than one ErrorInfo? Can you include a comment saying why that is not a problem?
google/cloud/grpc_error_delegate.cc, line 81 at r1 (raw file):
} void FillErrorInfo(google::rpc::Status const& status, ErrorInfo& error_info) {
Why the output argument? Can we just return error_info?
google/cloud/grpc_error_delegate.cc, line 122 at r1 (raw file):
} ErrorInfo error_info; FillErrorInfo(proto, error_info);
For a future PR: I think we should do the same thing for GCS, at least here:
Something like:
Status AsStatus(HttpResponse const& http_response) {
if (HttpStatusCode::kMinContinue <= http_response.status_code &&
http_response.status_code < HttpStatusCode::kMinSuccess) {
// We treat the 100s (e.g. 100 Continue) as OK results. They normally are
// ignored by libcurl, so we do not really expect to see them.
return Status{};
}
if (HttpStatusCode::kMinSuccess <= http_response.status_code &&
http_response.status_code < HttpStatusCode::kMinRedirects) {
// We treat the 200s as Okay results.
return Status{};
}
// Parse the payload as a JSON object, see https://cloud.google.com/apis/design/errors#http_mapping
auto parsed = nlohmann::json::parse(http_response.payload, nullptr, false);
ErrorInfo error_info = ExtractErrorInfo(parsed);
return Status(/* map HTTP code */, parsed.value("message", ""), error_info);google/cloud/status.h, line 70 at r1 (raw file):
* * @see * https://github.com/googleapis/googleapis/blob/master/google/rpc/error_details.proto
nit: I would use https://cloud.google.com/apis/design/errors#error_info
google/cloud/status.cc, line 159 at r1 (raw file):
auto const& error_info = s.error_info(); if (!error_info.reason().empty()) { os << "\n reason: " << error_info.reason();
Can we print the error in a single line (modulo the contents having \n in them)? It is super annoying to have multi-line error messages when using grep to parse them. Maybe format the ErrorInfo as as single-line JSON string, using the mapping from: https://cloud.google.com/apis/design/errors#http_mapping ?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @coryan)
google/cloud/grpc_error_delegate.cc, line 73 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
nit: what happens if there is more than one ErrorInfo? Can you include a comment saying why that is not a problem?
Done.
google/cloud/grpc_error_delegate.cc, line 81 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Why the output argument? Can we just return
error_info?
Just me being midway through a refactoring and not completing it. Oops. Yes, of course returning by value is better. Fixed.
google/cloud/grpc_error_delegate.cc, line 122 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
For a future PR: I think we should do the same thing for GCS, at least here:
Something like:
Status AsStatus(HttpResponse const& http_response) { if (HttpStatusCode::kMinContinue <= http_response.status_code && http_response.status_code < HttpStatusCode::kMinSuccess) { // We treat the 100s (e.g. 100 Continue) as OK results. They normally are // ignored by libcurl, so we do not really expect to see them. return Status{}; } if (HttpStatusCode::kMinSuccess <= http_response.status_code && http_response.status_code < HttpStatusCode::kMinRedirects) { // We treat the 200s as Okay results. return Status{}; } // Parse the payload as a JSON object, see https://cloud.google.com/apis/design/errors#http_mapping auto parsed = nlohmann::json::parse(http_response.payload, nullptr, false); ErrorInfo error_info = ExtractErrorInfo(parsed); return Status(/* map HTTP code */, parsed.value("message", ""), error_info);
This sounds like a good idea. I'm inclined to this the current PR close #7429, then file a new issue about adding "ErrorInfo" support to GCS, including the details from this comment, and linking to the original #7429
WDYT?
google/cloud/status.h, line 70 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
nit: I would use https://cloud.google.com/apis/design/errors#error_info
Better. Done.
google/cloud/status.cc, line 159 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Can we print the error in a single line (modulo the contents having
\nin them)? It is super annoying to have multi-line error messages when usinggrepto parse them. Maybe format the ErrorInfo as as single-line JSON string, using the mapping from: https://cloud.google.com/apis/design/errors#http_mapping ?
I can change to json if you think that's better, but first a few clarifying questions about the suggestion.
- Do you mean to print only the ErrorInfo stuff as json, and print the existing code and message not in json like it currently does? Or print all of it as json?
- Making this valid json would require that we escape quotes in the contained strings. I could probably stuff this stuff in an nlohmann json object and offload the work to that library. Is this what you had in mind?
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @devjgm)
google/cloud/grpc_error_delegate.cc, line 122 at r1 (raw file):
Previously, devjgm (Greg Miller) wrote…
This sounds like a good idea. I'm inclined to this the current PR close #7429, then file a new issue about adding "ErrorInfo" support to GCS, including the details from this comment, and linking to the original #7429
WDYT?
SGTM.
google/cloud/status.cc, line 159 at r1 (raw file):
Argh, quoting...
- Yes, I was thinking just the
ErrorInfoto format as json. - Using
nlohmann::jsonwould add a dep forcommonand thus all the libraries, not sure we want that.
Is this what you had in mind?
What I had in mind was ignoring the questions around quoting. It may still work, but I would not call it "JSON" anymore, something like:
std::ostream& operator<<(std::ostream& os, Status const& s) {
if (s.ok()) return os << StatusCode::kOk;
os << s.code << ": " << s.message();
auto const& ei = s.error_info();
if (ei.reason().empty() && ei.domain().empty() && ei.metadata.empty()) return os;
os << ", error_info={reason=" << ei.reason() << ", domain=" << ei.domain() << ", metadata={";
char const* sep = "";
for (auto const& e : ei.metadata()) {
os << sep << '"' << e.first << "\": \"" << e.second << '"';
sep = ", ";
}
return os << "}";
}Anyway, treat this as optional. I think it is better in that grep works, but worse in that it is harder to parse.
Fixes: googleapis#7429 This PR adds a `google::cloud::ErrorInfo` class, which mirrors [ErrorInfo][error-info-proto], and can also work with an equivalent type in a REST world. We expose programmatic access to this via the `g::c::Status::error_info()` accessor. We also log this data when streaming a `g::c::Status` if it's present. As of today, very few (if any) GCP services actually return this proto, so I don't expect to see it much yet. [error-info-proto]: https://github.com/googleapis/googleapis/blob/2e680c6be4b8e48c12f006b945b93edd41be8653/google/rpc/error_details.proto#L112
|
I restarted the |
Thanks. I'm still playing w/ one-line log formats, so I'll still be pushing another commit to build. |
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @coryan)
google/cloud/grpc_error_delegate.cc, line 122 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
SGTM.
google/cloud/status.cc, line 159 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Argh, quoting...
- Yes, I was thinking just the
ErrorInfoto format as json.- Using
nlohmann::jsonwould add a dep forcommonand thus all the libraries, not sure we want that.Is this what you had in mind?
What I had in mind was ignoring the questions around quoting. It may still work, but I would not call it "JSON" anymore, something like:
std::ostream& operator<<(std::ostream& os, Status const& s) { if (s.ok()) return os << StatusCode::kOk; os << s.code << ": " << s.message(); auto const& ei = s.error_info(); if (ei.reason().empty() && ei.domain().empty() && ei.metadata.empty()) return os; os << ", error_info={reason=" << ei.reason() << ", domain=" << ei.domain() << ", metadata={"; char const* sep = ""; for (auto const& e : ei.metadata()) { os << sep << '"' << e.first << "\": \"" << e.second << '"'; sep = ", "; } return os << "}"; }Anyway, treat this as optional. I think it is better in that
grepworks, but worse in that it is harder to parse.
Done. The output is one line now. This is more grepable, but it's less human readable. I don't think we need these to be perfectly parseable, because if programmatic access is needed, users can inspect the ErrorInfo object directly.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @devjgm)
google/cloud/status.cc, line 166 at r3 (raw file):
auto const& e = s.error_info(); if (e.reason().empty() && e.domain().empty() && e.metadata().empty()) return os;
nit: braces in this case?
I'm not sure I follow. You mean print |
|
You probably meant wrapping the if-body in braces. If so, done. |
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, all discussions resolved (waiting on @coryan)
google/cloud/status.cc, line 166 at r3 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
nit: braces in this case?
DOne
Yes, that is what I meant. Sorry if it was unclear. |
Fixes: #7429
This PR adds a
google::cloud::ErrorInfoclass, which mirrorsErrorInfo, and can also work with an equivalent
type in a REST world. We expose programmatic access to this via the
g::c::Status::error_info()accessor. We also log this data whenstreaming a
g::c::Statusif it's present.As of today, very few (if any) GCP services actually return this proto,
so I don't expect to see it much yet.
Alternative options:
ErrorInfointo it's ownerror_info.hfile. It's pretty closely related toStatus, so I'm OK w/ it where it is, but I'm also happy to move it if people think that's preferable.ErrorInfo. I chose something in this PR, but we can certainly consider other output formats.This change is