Skip to content

feat: get_ and tail_logs methods to Device #16

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.

Sign up for GitHub

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

Draft
cookjosh wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: get_ and tail_logs methods to Device #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions include/science/synapse/device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ class IDevice {
virtual auto stop(std::optional<std::chrono::milliseconds> timeout = std::nullopt) -> science::Status = 0;
virtual auto sockets() const -> const std::vector<synapse::NodeSocket>& = 0;
virtual auto uri() const -> const std::string& = 0;
virtual auto get_logs(
const std::string& log_level = "INFO",
std::optional<int64_t> since_ms = std::nullopt,
std::optional<int64_t> start_time_ns = std::nullopt,
std::optional<int64_t> end_time_ns = std::nullopt) -> std::vector<std::string> = 0;
virtual auto tail_logs(
const std::string& log_level = "INFO",
std::optional<std::chrono::milliseconds> timeout = std::nullopt) -> std::vector<std::string> = 0;
};

/**
Expand Down Expand Up @@ -66,6 +74,32 @@ class Device : public IDevice {
*/
[[nodiscard]] auto uri() const -> const std::string&;

/**
* Get the logs from the device.
*
* @param log_level Minimum log level to retrieve (default: INFO)
* @param since_ms Get logs from this many milliseconds ago (optional)
* @param start_time_ns Start time in nanoseconds since epoch (optional)
* @param end_time_ns End time in nanoseconds since epoch (optional)
* @return std::vector<std::string> containing log entries
*/
[[nodiscard]] auto get_logs(
Copy link
Contributor

@antoniaelsen antoniaelsen Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For both methods, could you return a science::Status -- its our convention for reporting any errors (rather than throwing)

You could either return a tuple [Status, string], or, more c++-like, add arguments at the end of each function's signature for outputs, e.g.
get_logs(const std::string& log_level, ..., std::vector<std::string>* output)

const std::string& log_level = "INFO",
std::optional<int64_t> since_ms = std::nullopt,
std::optional<int64_t> start_time_ns = std::nullopt,
std::optional<int64_t> end_time_ns = std::nullopt) -> std::vector<std::string>;

/**
* Tail the logs from the device.
*
* @param log_level Minimum log level to retrieve (default: INFO)
* @param timeout Timeout for the request (optional)
* @return std::vector<std::string> containing the most recent log entries
*/
[[nodiscard]] auto tail_logs(
const std::string& log_level = "INFO",
Copy link
Contributor

@antoniaelsen antoniaelsen Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add a LogLevel enum or use the existing proto one rather than accepting raw strings.

I like the default value!

std::optional<std::chrono::milliseconds> timeout = std::nullopt) -> std::vector<std::string>;

private:
std::string uri_;
std::shared_ptr<grpc::Channel> channel_;
Expand All @@ -74,6 +108,8 @@ class Device : public IDevice {
std::vector<synapse::NodeSocket> sockets_;

[[nodiscard]] auto handle_status_response(const synapse::Status& status) -> science::Status;

[[nodiscard]] auto log_level_to_pb(const std::string& level) -> synapse::LogLevel;
};

} // namespace synapse
81 changes: 81 additions & 0 deletions src/science/synapse/device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,78 @@ auto Device::uri() const -> const std::string& {
return uri_;
}

auto Device::get_logs(
const std::string& log_level,
std::optional<int64_t> since_ms,
std::optional<int64_t> start_time_ns,
std::optional<int64_t> end_time_ns,
std::optional<std::chrono::milliseconds> timeout) -> std::vector<std::string> {

synapse::LogQueryRequest request;
request.set_min_level(log_level_to_pb(log_level));

if (since_ms.has_value()) {
request.set_since_ms(since_ms.value());
} else {
if (start_time_ns.has_value()) {
Copy link
Contributor

@antoniaelsen antoniaelsen Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

while we're here, mind checking that start_time_ns is less than end_time_ns, and if it's not, return an error science::status?

request.set_start_time_ns(start_time_ns.value());
}
if (end_time_ns.has_value()) {
request.set_end_time_ns(end_time_ns.value());
}
}

grpc::ClientContext context;
if (timeout.has_value()) {
context.set_deadline(std::chrono::system_clock::now() + timeout.value());
}

synapse::LogQueryResponse response;
std::vector<std::string> logs;

grpc::Status status = rpc_->GetLogs(&context, request, &response);

if (status.ok()) {
for (const auto& entry : response.entries()) {
logs.push_back(entry.message());
}
} else {
std::cerr << "Error getting logs: " << status.error_message() << std::endl;
Copy link
Contributor

@antoniaelsen antoniaelsen Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't print an error (because consumers won't be able to turn these logs off) -- instead report this error message in a returned science::status (and preferably don't capitalize the first string so they're easier to chain)

}

return logs;
}

auto Device::tail_logs(
Copy link
Contributor

@antoniaelsen antoniaelsen Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm realizing that the gRPC client in this device class is sync, when we probably want async (whoops). Ideally a consumer could keep a 'tail_logs' call open & running while they make other device calls, but with this current implementation that's not possible. I'll add a backlog task to change the client from sync to async -- for now, lets not include this

const std::string& log_level,
std::optional<std::chrono::milliseconds> timeout) -> std::vector<std::string> {

synapse::TailLogsRequest request;
request.set_min_level(log_level_to_pb(log_level));

grpc::ClientContext context;
if (timeout.has_value()) {
context.set_deadline(std::chrono::system_clock::now() + timeout.value());
}

std::vector<std::string> logs;

std::unique_ptr<grpc::ClientReader<synapse::LogEntry>> reader(
rpc_->TailLogs(&context, request));

synapse::LogEntry entry;
while (reader->Read(&entry)) {
logs.push_back(entry.message());
}

grpc::Status status = reader->Finish();
if (!status.ok()) {
std::cerr << "Error tailing logs: " << status.error_message() << std::endl;
}

return logs;
}

auto Device::handle_status_response(const synapse::Status& status) -> science::Status {
if (status.code() != synapse::StatusCode::kOk) {
return {
Expand All @@ -177,4 +249,13 @@ auto Device::handle_status_response(const synapse::Status& status) -> science::S
return {};
}

auto Device::log_level_to_pb(const std::string& level) -> synapse::LogLevel {
if (level == "DEBUG") return synapse::LogLevel::LOG_LEVEL_DEBUG;
if (level == "INFO") return synapse::LogLevel::LOG_LEVEL_INFO;
if (level == "WARNING") return synapse::LogLevel::LOG_LEVEL_WARNING;
if (level == "ERROR") return synapse::LogLevel::LOG_LEVEL_ERROR;
if (level == "CRITICAL") return synapse::LogLevel::LOG_LEVEL_CRITICAL;
return synapse::LogLevel::LOG_LEVEL_UNKNOWN;
}

} // namespace synapse