feat: support truncation of strings/bytes fields in protobuf logging #1192
Conversation
Extend `internal::ConnectionImpl` to hold the length to which protobuf string/bytes fields should be truncated during gRPC request/response logging, and pass it along to the logging wrappers. There it is used in a special `DebugString()` implementation. For now we just set it to a fixed value (256) until it is clear that this is a direction we want to head, and until we decide how it should be configured.
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #1192 +/- ##
==========================================
+ Coverage 95.7% 95.71% +0.01%
==========================================
Files 174 180 +6
Lines 13601 13686 +85
==========================================
+ Hits 13017 13100 +83
- Misses 584 586 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you for doing this. Feels like we should have at least one test to verify the the truncation actually happens?
Reviewed 21 of 21 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @devbww)
google/cloud/spanner/connection_options.h, line 125 at r1 (raw file):
/// Return the maximum length for string/bytes fields when tracing RPCs. std::int64_t max_text_proto_string_length() const {
nit: std::int64_t seems like an overkill for this, but probably more compatible with std::size_t.
google/cloud/spanner/connection_options.cc, line 53 at r1 (raw file):
} } // TODO(XXX): Provide some way to override this from the environment.
Reminder: create an actual bug number to reference here.
google/cloud/spanner/internal/log_wrapper.h, line 23 at r1 (raw file):
#include "google/cloud/log.h" #include "google/cloud/status_or.h" #include <google/protobuf/io/zero_copy_stream_impl_lite.h>
Do we use that?
google/cloud/spanner/internal/log_wrapper.h, line 35 at r1 (raw file):
template <typename Message> std::string DebugString(Message const& m,
I do not think this needs to be a template, it can take a google::protobuf::Message const& m parameter?
google/cloud/spanner/internal/log_wrapper.h, line 40 at r1 (raw file):
{ google::protobuf::io::StringOutputStream os(&str); google::protobuf::TextFormat::Printer p;
FYI: There is also a Printer::PrintToString member function, would that not work?
There are also a number of other formatting options, including SetSingleLineMode() (which seems appropriate), UseShortRepeatedPrimitives, and others.
For this PR, I think setting SetSingleLineMode() is a good idea. For a future PR, maybe receive a struct with more options so we can extend the log formatting (seems like overkill, but had to be said).
google/cloud/spanner/internal/logging_spanner_stub.cc, line 17 at r1 (raw file):
#include "google/cloud/spanner/internal/logging_spanner_stub.h" #include "google/cloud/spanner/internal/log_wrapper.h" #include "google/cloud/internal/invoke_result.h"
Thanks!
- Support multiple tracing options, with useful defaults
- Override tracing options from `${GOOGLE_CLOUD_CPP_TRACING_OPTIONS}`
- Simplify `google::protobuf::TextFormat::Printer` usage
- Add tests
There was a problem hiding this comment.
Reviewable status: 1 of 30 files reviewed, 4 unresolved discussions (waiting on @coryan)
google/cloud/spanner/connection_options.h, line 125 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
nit:
std::int64_tseems like an overkill for this, but probably more compatible withstd::size_t.
I chose std::int64_t to match the argument type to SetTruncateStringFieldLongerThan(). That avoid any conversions.
google/cloud/spanner/connection_options.cc, line 53 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Reminder: create an actual bug number to reference here.
It is no longer a TODO.
google/cloud/spanner/internal/log_wrapper.h, line 23 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Do we use that?
We did ... for google::protobuf::io::StringOutputStream. But your suggestion to use the PrintToString() wrapper obviates that. Thanks.
google/cloud/spanner/internal/log_wrapper.h, line 35 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I do not think this needs to be a template, it can take a
google::protobuf::Message const& mparameter?
Done.
google/cloud/spanner/internal/log_wrapper.h, line 40 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
FYI: There is also a
Printer::PrintToStringmember function, would that not work?There are also a number of other formatting options, including
SetSingleLineMode()(which seems appropriate),UseShortRepeatedPrimitives, and others.For this PR, I think setting
SetSingleLineMode()is a good idea. For a future PR, maybe receive astructwith more options so we can extend the log formatting (seems like overkill, but had to be said).
Done.
There was a problem hiding this comment.
Reviewed 29 of 29 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
google/cloud/spanner/connection_options.h, line 125 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
I chose
std::int64_tto match the argument type toSetTruncateStringFieldLongerThan(). That avoid any conversions.
Ah, that makes sense.
…oogleapis/google-cloud-cpp-spanner#1192) Add support for formatting options used during RPC request/response tracing. The options, and their default values, are currently: - single_line_mode=on - use_short_repeated_primitives=on - truncate_string_field_longer_than=128 `${GOOGLE_CLOUD_CPP_TRACING_OPTIONS}` can to set to a comma-separated list of "key=value" pairs to override these at runtime.
Extend
internal::ConnectionImplto hold the length to which protobufstring/bytes fields should be truncated during gRPC request/response
logging, and pass it along to the logging wrappers. There it is used
in a special
DebugString()implementation.For now we just set it to a fixed value (256) until it is clear that
this is a direction we want to head, and until we decide how it should
be configured.
This change is