Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace LogEntry Rust struct with a protobuf object #568

Closed
tiziano88 opened this issue Feb 6, 2020 · 4 comments · Fixed by #697
Closed

Replace LogEntry Rust struct with a protobuf object #568

tiziano88 opened this issue Feb 6, 2020 · 4 comments · Fixed by #697
Assignees

Comments

@tiziano88
Copy link
Collaborator

/// An object representing a log entry. Currently just a wrapper around a string, but it may be
/// extended in the future with additional fields, e.g. level or file / line information, though
/// that would probably require defining a cross-language schema such as protobuf or FIDL for it,
/// rather than just a Rust struct.
struct LogEntry {
message: String,
}

Currently LogEntry is a Rust struct wrapping a string, and on the C++ side of the logging pseudo-node, this is de-serialized as a string, but there is no "schema" to keep the two sides in sync, especially if we wanted to add more fields to it (e.g. log level).

@daviddrysdale : it looks like protobuf is going to be the de facto IDL, at least for the time being, so perhaps we should replace this object (and others) with corresponding protobuf ones. What are your thoughts?

@daviddrysdale
Copy link
Contributor

Yeah, I think that makes sense for now. Something like the following?

message LogMessage {
  string file = 1;
  int32 line = 2;
  int32 level = 3;
  string message = 4;
}

@tiziano88
Copy link
Collaborator Author

For reference, this is the corresponding Rust struct: https://docs.rs/log/0.4.10/log/struct.Record.html (not that we have to necessarily stick to that though).

@tiziano88
Copy link
Collaborator Author

Assigning to @rbehjati to investigate, I think it makes sense to go ahead with that proto definition, which perhaps should live near or inside oak_abi (especially after #666 ).

FYI the log crate is in the process of adding key/value support (see https://docs.rs/log/0.4.10/log/kv/index.html ), so we may look into supporting that in the future.

rbehjati added a commit to rbehjati/oak that referenced this issue Mar 11, 2020
* 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
@rbehjati
Copy link
Contributor

The change list in (#697) relies on rust-protobuf for generating rust structs from proto file. The use of rust-protobuf in the entire crate will later be replaced with prost in a separate task.

rbehjati added a commit to rbehjati/oak that referenced this issue Mar 13, 2020
* 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
rbehjati added a commit to rbehjati/oak that referenced this issue Mar 13, 2020
* 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
rbehjati added a commit to rbehjati/oak that referenced this issue Mar 13, 2020
* 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
rbehjati added a commit to rbehjati/oak that referenced this issue Mar 13, 2020
* 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
rbehjati added a commit to rbehjati/oak that referenced this issue Mar 13, 2020
* 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
rbehjati added a commit that referenced this issue Mar 13, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants