Replace LogEntry struct with LogMessage protobuf (#568) #697

rbehjati · 2020-03-11T20:20:13Z

Struct LogEntry in sdk/rust/oak/src/logger/mod.rs is removed
New file oak/proto/logger.proto containing LogMessage is added
OakChannelLogger is updated to send LogMessage instead of string
LoggingNode is updated to handle LogMessage instances

Fixes #568

Checklist

Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
- I have written tests that cover the code changes.
- I have checked that these tests are run by Cloudbuild
- I have updated documentation accordingly.
- I have raised an issue to
  cover any TODOs and/or unfinished work.
Pull request includes prototype/experimental work that is under
construction.

tiziano88

Thanks, looking good!

tiziano88 · 2020-03-11T20:50:17Z

oak/proto/logger.proto

+
+// Logging levels as defined in https://docs.rs/log/0.4.10/log/enum.Level.html.
+enum Level {
+  ERROR = 0;


It is good practice to have a default unknown value for enums, so that if a sender forgets to set it, or sets it to a value that the receiver does not recognize, it can be detected. See https://engdoc.corp.google.com/eng/doc/devguide/proto/index.md?cl=head#do-include-an-unknown-value-in-an-enum

Added UNKNOWN_LEVEL as the default value.

tiziano88 · 2020-03-11T20:50:29Z

oak/proto/logger.proto

+  INFO = 2;
+  DEBUG = 3;
+  TRACE = 4;
+}


Missing newline.

tiziano88 · 2020-03-11T20:51:17Z

oak/proto/logger.proto

+
+syntax = "proto3";
+
+package oak.logging;


I would rename this to oak.log and also the file name to log.proto.

tiziano88 · 2020-03-11T21:04:27Z

oak/server/logging_node.h

@@ -32,6 +32,7 @@ class LoggingNode final : public NodeThread {
  explicit LoggingNode(BaseRuntime* runtime, const std::string& name) : NodeThread(runtime, name) {}

 private:
+  static const std::string log_levels[5];


Not sure why this needs to be a member of this class, as opposed to just a constant in an unnamed namespace?

Fixed. Found that protobuf compiler generates a function that returns the enum name corresponding to a numeric value.

tiziano88 · 2020-03-11T21:07:55Z

sdk/rust/oak/src/logger/mod.rs

+            cached_size: ::std::default::Default::default(),
+            unknown_fields: ::std::default::Default::default(),


You should use the ..Default::default() idiom to ensure that all fields are filled in, see elsewhere.

oak/examples/hello_world/module/rust/build.rs

Lines 17 to 24 in 4ad9891

fn main() {

oak_utils::run_protoc_rust_grpc(protoc_rust_grpc::Args {

out_dir: "src/proto",

input: &["../../proto/hello_world.proto"],

includes: &["../../proto", "../../../../third_party"],

rust_protobuf: true, // also generate protobuf messages, not just services

..Default::default()

})

Nice idiom. Thanks. Fixed :)

tiziano88 · 2020-03-11T21:12:07Z

sdk/rust/oak/src/logger/mod.rs

-                record.args()
-            ),
+        let log_msg = crate::proto::logger::LogMessage {
+            file: record.file().unwrap_or_default().to_string(),


Perhaps make this more explicit, e.g. unwrap_or("<unknown>") or similar.

Changed it to file: record.file().unwrap_or("<unknown-file>").to_string(),.
The next line also calls unwrap_or_default(), but on an int value. Do we need an explicit default value there too?

I guess that's enough, obviously it will show up as 0 in the output, but I guess that's fine for now.

tiziano88 · 2020-03-11T21:12:52Z

oak/server/logging_node.cc


 namespace oak {

+const std::string LoggingNode::log_levels[5] = {"ERROR", "WARN", "INFO", "DEBUG", "TRACE"};


Is there any way of getting this string representation out of an actual enum value?

Yes. Fixed using Level_Name function.

tiziano88 · 2020-03-11T21:14:47Z

oak/server/logging_node.cc

@@ -47,8 +50,12 @@ void LoggingNode::Run(Handle handle) {
      if (result.msg == nullptr) {
        break;
      }
+      oak::logging::LogMessage log_msg;
+      std::string msg_str(result.msg->data.begin(), result.msg->data.end());


It's slightly annoying to have to make this extra copy, is there any way to parse the protobuf message directly from the bytes field? Failing that, perhaps creating a string_view would at least avoid the copy, even though it would still require an explicit conversion here?

I could not make it work with string_view. Apparently the compiler does not recognize it!
This is the code I used:

oak::log::LogMessage log_msg; std::string_view str(result.msg->data.data(), result.msg->data.size()); log_msg.ParseFromString(str.data());

And the error message is: no member named 'string_view' in namespace 'std'.

I also tried to parse the protobuf directly: log_msg.ParseFromString(result.msg->data.data());, but it results in memory corruption, when running the examples with -s asan. We need a null-terminated string here, and for that we have to make a copy. Can you think of a work around?

I think string_view is in absl, not in std yet (but i have not checked).

Anyways, it seems maybe https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message_lite#MessageLite.ParseFromArray would do the trick here?

Fixed using ParseFromArray. Thanks for suggesting it @tiziano88.

tiziano88

Let me know when I should take another look, thanks!

tiziano88 · 2020-03-13T17:48:14Z

oak/server/logging_node.cc

@@ -47,8 +48,11 @@ void LoggingNode::Run(Handle handle) {
      if (result.msg == nullptr) {
        break;
      }
+      oak::log::LogMessage log_msg;
+      log_msg.ParseFromArray(result.msg->data.data(), result.msg->data.size());


I think this parsing may fail, what happens if so? Does this return a status / error code?

tiziano88 · 2020-03-13T17:48:33Z

oak/proto/log.proto

+
+package oak.log;
+
+message LogMessage {


Could you add a comment explaining what this is used for?

rbehjati · 2020-03-13T18:30:58Z

oak/proto/log.proto

+
+package oak.log;
+
+message LogMessage {


rbehjati · 2020-03-13T18:36:11Z

oak/server/logging_node.cc

+        LOG(INFO) << "{" << name_ << "} "
+                  << "LOG: " << oak::log::Level_Name(log_msg.level()) << " " << log_msg.file()
+                  << ":" << log_msg.line() << ": " << log_msg.message();
+      } else {


I think this parsing may fail, what happens if so? Does this return a status / error code?

Error message if the parse fails.

oak/proto/log.proto

oak/server/logging_node.cc

googlebot · 2020-03-13T20:30:46Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-03-13T20:30:46Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

* LogEntry in sdk/rust/oak/src/logger/mod.rs is removed * oak/proto/logger.proto containing LogMessage is added * OakChannelLogger is updated to send LogMessage instead of string * LoggingNode is updated to handle LogMessage instances

googlebot · 2020-03-13T20:35:28Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

daviddrysdale · 2020-03-16T12:45:15Z

oak/server/logging_node.cc

@@ -47,8 +48,16 @@ void LoggingNode::Run(Handle handle) {
      if (result.msg == nullptr) {
        break;
      }
-      LOG(INFO) << "{" << name_ << "} "


Ah, there are actually two runtime implementations of the logging pseudo-Node – you also need to update oak/server/rust/oak_runtime/src/node/logger.rs to cope with proto-encoded logging messages.

Oh! Thanks for letting me know. I'll fix it :)

googlebot added the cla: yes label Mar 11, 2020

rbehjati requested a review from tiziano88 March 11, 2020 20:20

rbehjati mentioned this pull request Mar 11, 2020

Replace LogEntry Rust struct with a protobuf object #568

Closed

tiziano88 reviewed Mar 11, 2020

View reviewed changes

rbehjati force-pushed the feature/oak-568 branch 2 times, most recently from 74bd183 to f451143 Compare March 13, 2020 15:48

rbehjati requested a review from tiziano88 March 13, 2020 16:25

tiziano88 reviewed Mar 13, 2020

View reviewed changes

rbehjati force-pushed the feature/oak-568 branch from f451143 to fea8a72 Compare March 13, 2020 16:53

rbehjati requested a review from tiziano88 March 13, 2020 17:29

tiziano88 approved these changes Mar 13, 2020

View reviewed changes

rbehjati force-pushed the feature/oak-568 branch from fea8a72 to 6c38706 Compare March 13, 2020 18:30

rbehjati commented Mar 13, 2020

View reviewed changes

tiziano88 approved these changes Mar 13, 2020

View reviewed changes

oak/proto/log.proto Outdated Show resolved Hide resolved

oak/server/logging_node.cc Outdated Show resolved Hide resolved

googlebot added cla: no and removed cla: yes labels Mar 13, 2020

rbehjati force-pushed the feature/oak-568 branch from f23a316 to c92cf01 Compare March 13, 2020 20:35

googlebot added cla: yes and removed cla: no labels Mar 13, 2020

rbehjati merged commit 54a31e7 into project-oak:master Mar 13, 2020

daviddrysdale reviewed Mar 16, 2020

View reviewed changes

rbehjati deleted the feature/oak-568 branch March 16, 2020 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace LogEntry struct with LogMessage protobuf (#568) #697

Replace LogEntry struct with LogMessage protobuf (#568) #697

rbehjati commented Mar 11, 2020

tiziano88 left a comment

tiziano88 Mar 11, 2020

rbehjati Mar 13, 2020

tiziano88 Mar 11, 2020

rbehjati Mar 13, 2020

tiziano88 Mar 11, 2020

rbehjati Mar 13, 2020

tiziano88 Mar 11, 2020

rbehjati Mar 13, 2020

tiziano88 Mar 11, 2020

rbehjati Mar 13, 2020 •

edited

Loading

tiziano88 Mar 11, 2020

rbehjati Mar 13, 2020

tiziano88 Mar 13, 2020

tiziano88 Mar 11, 2020

rbehjati Mar 13, 2020

tiziano88 Mar 11, 2020

rbehjati Mar 13, 2020

tiziano88 Mar 13, 2020

rbehjati Mar 13, 2020

tiziano88 left a comment

tiziano88 Mar 13, 2020

tiziano88 Mar 13, 2020

rbehjati Mar 13, 2020

rbehjati Mar 13, 2020

rbehjati Mar 13, 2020

googlebot commented Mar 13, 2020

googlebot commented Mar 13, 2020

googlebot commented Mar 13, 2020

daviddrysdale Mar 16, 2020

rbehjati Mar 16, 2020

		cached_size: ::std::default::Default::default(),
		unknown_fields: ::std::default::Default::default(),

	fn main() {
	oak_utils::run_protoc_rust_grpc(protoc_rust_grpc::Args {
	out_dir: "src/proto",
	input: &["../../proto/hello_world.proto"],
	includes: &["../../proto", "../../../../third_party"],
	rust_protobuf: true, // also generate protobuf messages, not just services
	..Default::default()
	})


		namespace oak {

		const std::string LoggingNode::log_levels[5] = {"ERROR", "WARN", "INFO", "DEBUG", "TRACE"};


		syntax = "proto3";

		package oak.logging;

Replace LogEntry struct with LogMessage protobuf (#568) #697

Replace LogEntry struct with LogMessage protobuf (#568) #697

Conversation

rbehjati commented Mar 11, 2020

Checklist

tiziano88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbehjati Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiziano88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Mar 13, 2020

googlebot commented Mar 13, 2020

googlebot commented Mar 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbehjati Mar 13, 2020 •

edited

Loading