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

Add custom OpenTelemetrySpanExt trait #700

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
12 changes: 12 additions & 0 deletions Cargo.lock

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

4 changes: 2 additions & 2 deletions crates/block-producer/src/block_builder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::{
};

use futures::FutureExt;
use miden_node_utils::tracing::{OpenTelemetrySpanExt, OtelStatus};
use miden_node_utils::tracing::OpenTelemetrySpanExt;
use miden_objects::{
account::AccountId,
batch::ProvenBatch,
Expand Down Expand Up @@ -106,7 +106,7 @@ impl BlockBuilder {
.and_then(|proven_block| async { self.inject_failure(proven_block) })
.and_then(|proven_block| self.commit_block(mempool, proven_block))
// Handle errors by propagating the error to the root span and rolling back the block.
.inspect_err(|err| Span::current().set_status(OtelStatus::Error { description: format!("{err:?}").into() }))
.inspect_err(|err| Span::current().set_error(err))
.or_else(|_err| self.rollback_block(mempool).never_error())
// Error has been handled, this is just type manipulation to remove the result wrapper.
.unwrap_or_else(|_| ())
Expand Down
1 change: 1 addition & 0 deletions crates/utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ opentelemetry = { version = "0.28" }
opentelemetry-otlp = { version = "0.28", default-features = false, features = ["grpc-tonic", "tls-roots", "trace"] }
opentelemetry_sdk = { version = "0.28", features = ["rt-tokio"] }
rand = { workspace = true }
sealed = { version = "0.6" }
serde = { version = "1.0", features = ["derive"] }
thiserror = { workspace = true }
tonic = { workspace = true }
Expand Down
29 changes: 25 additions & 4 deletions crates/utils/src/tracing/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,27 @@
pub mod grpc;

// Re-export useful traits for open-telemetry traces. This avoids requiring other crates from
// importing that family of crates directly.
pub use opentelemetry::trace::Status as OtelStatus;
pub use tracing_opentelemetry::OpenTelemetrySpanExt;
use opentelemetry::trace::Status as OtelStatus;
use opentelemetry::{Key, Value};
Copy link
Author

Choose a reason for hiding this comment

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

@Mirko-von-Leipzig in the issue you mentioned:

We can make set_attribute more ergonomic by adding a secondary trait ToValue which we can then implement for foreign types. i.e. instead of set_attribute("hash", digest.to_hex()), we can just do set_attribute("hash", &digest).

I was wondering if this is worthwhile considering that crates can already impl From<...> for opentelemetry::Value for their types and rely on the trait as is , value: impl Into<Value>);.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of our core types come from the crates from miden-base - and I don't think introducing open-telemetry there is a reasonable expectation. Though it could make sense if we really want to standardize on it everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually here we go: https://docs.rs/opentelemetry_sdk/latest/opentelemetry_sdk/testing/trace/index.html

They do actually have test infra :)

Copy link
Author

Choose a reason for hiding this comment

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

Just checking you meant that this is relevant for this PR and not the other trace-related PR #698.

TestSpan implements trace::Span so I would have to change our Ext trait to be over impl<S: trace::Span + tracing_opentelemetry::OpenTelemetrySpanExt> and then make sure the tracing_opentelemetry::OpenTelemetrySpanExt applies to TestSpan. Unless I am missing something about how to using those testing utilities.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right; yeah this was intended for their 😅 Will cross-post just for traceability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Yes good point - we can impl<S: OpenTelemetrySpanExt> because it in turn is implemented for T: tracing::Span. So we just piggy back on theirs? Alternatively we keep as is and add another impl for the TestSpan specifically.

I'd do the former, and then see how it works out in the other PR?

use sealed::sealed;

/// Utility functions based on [`tracing_opentelemetry::OpenTelemetrySpanExt`].
#[sealed]
pub trait OpenTelemetrySpanExt {
fn set_attribute(&self, key: impl Into<Key>, value: impl Into<Value>);
fn set_error(&self, err: &dyn std::error::Error);
}

#[sealed]
impl OpenTelemetrySpanExt for tracing::Span {
/// Sets an attribute on `Span`.
fn set_attribute(&self, key: impl Into<Key>, value: impl Into<Value>) {
tracing_opentelemetry::OpenTelemetrySpanExt::set_attribute(self, key, value);
}
/// Sets a status on `Span` based on an error.
fn set_error(&self, err: &dyn std::error::Error) {
tracing_opentelemetry::OpenTelemetrySpanExt::set_status(
self,
OtelStatus::Error { description: format!("{err:?}").into() },
);
}
}
Loading