Skip to content

Commit

Permalink
Inline the sole consumer of omicron_common::generate_logging_api (#6707)
Browse files Browse the repository at this point in the history
This can simplify omicron-common at least a little by removing its
direct dependency on progenitor (and all its sundry transitive
dependencies).
  • Loading branch information
ahl authored Sep 27, 2024
1 parent d7235b8 commit ea2f4d2
Show file tree
Hide file tree
Showing 6 changed files with 25 additions and 38 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 1 addition & 3 deletions clients/gateway-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
pub use gateway_messages::SpComponent;

// We specifically want to allow consumers, such as `wicketd`, to embed
// inventory datatypes into their own APIs, rather than recreate structs. For
// this purpose, we copied the `omicron_common::generate_logging_api!` macro
// body and added a `derives = [schemars::JsonSchema]` line at the bottom.
// inventory datatypes into their own APIs, rather than recreate structs.
//
// We did not add this functionality to `omicron_common` because, in the common
// case, we want to prohibit users from accidentally exposing implementation
Expand Down
15 changes: 14 additions & 1 deletion clients/oximeter-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,20 @@

//! Interface for API requests to an Oximeter metric collection server
omicron_common::generate_logging_api!("../../openapi/oximeter.json");
progenitor::generate_api!(
spec = "../../openapi/oximeter.json",
inner_type = slog::Logger,
pre_hook = (|log: &slog::Logger, request: &reqwest::Request| {
slog::debug!(log, "client request";
"method" => %request.method(),
"uri" => %request.url(),
"body" => ?&request.body(),
);
}),
post_hook = (|log: &slog::Logger, result: &Result<_, _>| {
slog::debug!(log, "client response"; "result" => ?result);
}),
);

impl omicron_common::api::external::ClientError for types::Error {
fn message(&self) -> String {
Expand Down
1 change: 0 additions & 1 deletion common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ thiserror.workspace = true
tokio = { workspace = true, features = ["full"] }
uuid.workspace = true
parse-display.workspace = true
progenitor.workspace = true
progenitor-client.workspace = true
omicron-workspace-hack.workspace = true
once_cell.workspace = true
Expand Down
22 changes: 10 additions & 12 deletions common/src/api/external/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,25 +540,23 @@ pub trait ClientError: std::fmt::Debug {
// external client, others may require, for example, retries with an alternate
// service instance or additional interpretation to sanitize the output error.
// This should be removed to avoid leaking data.
impl<T: ClientError> From<progenitor::progenitor_client::Error<T>> for Error {
fn from(e: progenitor::progenitor_client::Error<T>) -> Self {
impl<T: ClientError> From<progenitor_client::Error<T>> for Error {
fn from(e: progenitor_client::Error<T>) -> Self {
match e {
// For most error variants, we delegate to the display impl for the
// Progenitor error type, but we pick apart an error response more
// carefully.
progenitor::progenitor_client::Error::InvalidRequest(_)
| progenitor::progenitor_client::Error::CommunicationError(_)
| progenitor::progenitor_client::Error::InvalidResponsePayload(
..,
)
| progenitor::progenitor_client::Error::UnexpectedResponse(_)
| progenitor::progenitor_client::Error::InvalidUpgrade(_)
| progenitor::progenitor_client::Error::ResponseBodyError(_)
| progenitor::progenitor_client::Error::PreHookError(_) => {
progenitor_client::Error::InvalidRequest(_)
| progenitor_client::Error::CommunicationError(_)
| progenitor_client::Error::InvalidResponsePayload(..)
| progenitor_client::Error::UnexpectedResponse(_)
| progenitor_client::Error::InvalidUpgrade(_)
| progenitor_client::Error::ResponseBodyError(_)
| progenitor_client::Error::PreHookError(_) => {
Error::internal_error(&e.to_string())
}
// This error represents an expected error from the remote service.
progenitor::progenitor_client::Error::ErrorResponse(rv) => {
progenitor_client::Error::ErrorResponse(rv) => {
let message = rv.message();

match rv.status() {
Expand Down
20 changes: 0 additions & 20 deletions common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,26 +34,6 @@ pub mod zpool_name;

pub use update::hex_schema;

#[macro_export]
macro_rules! generate_logging_api {
($path:literal) => {
progenitor::generate_api!(
spec = $path,
inner_type = slog::Logger,
pre_hook = (|log: &slog::Logger, request: &reqwest::Request| {
slog::debug!(log, "client request";
"method" => %request.method(),
"uri" => %request.url(),
"body" => ?&request.body(),
);
}),
post_hook = (|log: &slog::Logger, result: &Result<_, _>| {
slog::debug!(log, "client response"; "result" => ?result);
}),
);
};
}

/// A type that allows adding file and line numbers to log messages
/// automatically. It should be instantiated at the root logger of each
/// executable that desires this functionality, as in the following example.
Expand Down

0 comments on commit ea2f4d2

Please sign in to comment.