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

feat(tenderdash-abci): deterministic request_id #38

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
[workspace]

resolver = "2"
members = ["abci", "proto-compiler", "proto"]
4 changes: 2 additions & 2 deletions abci/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ server = [
crypto = ["dep:lhash"]
tcp = ["server"]
unix = ["server"]
tracing-span = ["dep:uuid"]
tracing-span = ["dep:uuid", "dep:lhash", "lhash/md5"]

[[example]]
name = "echo_socket"
required-features = ["server"]

[dependencies]
uuid = { version = "1.4.1", features = ["v4", "fast-rng"], optional = true }
uuid = { version = "1.4.1", default-features = false, optional = true }
tenderdash-proto = { version = "0.13.0", path = "../proto" }
bytes = { version = "1.0" }
prost = { version = "0.11" }
Expand Down
2 changes: 1 addition & 1 deletion abci/src/application.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ pub trait RequestDispatcher {
impl<A: Application> RequestDispatcher for A {
fn handle(&self, request: abci::Request) -> Option<abci::Response> {
#[cfg(feature = "tracing-span")]
let _span = super::tracing_span::span(request.clone().value?);
let _span = super::tracing_span::span(&request);
tracing::trace!(?request, "received ABCI request");

let response: response::Value = match request.value? {
Expand Down
37 changes: 24 additions & 13 deletions abci/src/tracing_span.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use tenderdash_proto::abci::request::Value;
use prost::Message;
use tenderdash_proto::abci::{self, request::Value};
use tracing::Level;

const SPAN_NAME: &str = "abci";
Expand All @@ -20,16 +21,18 @@ macro_rules! block_span {
///
/// This function creates a new `tracing::span::EnteredSpan` based on the
/// provided request. It uses the request to determine the endpoint and includes
/// a unique request ID in the span.
/// a request ID in the span.
///
/// Request ID is deterministic and is based on the request value. It is
/// not guaranteed to be unique, as the same request can be sent multiple times.
/// However, it should be the same on all nodes for the same request.
///
/// The level of the span is set to ERROR, so it will be included on all log
/// levels.
///
/// # Arguments
///
/// * `request` - A value that can be converted into a `Value`. Depending on the
/// specific variant of `Value`, additional information like height, round, or
/// path might be included in the span.
/// * `request` - request to create a span for.
///
/// # Returns
///
Expand All @@ -38,20 +41,28 @@ macro_rules! block_span {
/// # Examples
///
/// ```
/// # use tenderdash_proto::abci::{RequestInfo, request};
/// # use tenderdash_proto::abci::{RequestInfo, Request, request::Value};
/// # use tenderdash_abci::tracing_span::span;
///
/// let request = request::Value::Info(RequestInfo::default());
/// let span = span(request);
/// let request = Request {
/// value: Some(Value::Info(Default::default())),
/// };
/// let span = span(&request);
/// ```
pub fn span<T>(request: T) -> tracing::span::EnteredSpan
pub fn span(request: &abci::Request) -> tracing::span::EnteredSpan
where
T: Into<Value>,
{
let value = request.into();
let value = request.value.as_ref().expect("request value is missing");

// we use md5 as we need 16-byte request id for uuid, and it doesn't have to be
Copy link
Member

Choose a reason for hiding this comment

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

This approach has downsides, especially if we want to filter by specific requests in elastic. There are endpoints like info, query, etc. where we don't have much difference so they will have the same ID.
We could have both: unique request ID and md5 so we will benefit from both depending on what we need.

That would be great if we could use the same request ID with tenderdash so we can easily filter related events.

// cryptographically secure
let mut md5 = lhash::Md5::new();
Copy link
Member

Choose a reason for hiding this comment

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

I think it's pretty expensive, especially for the process and prepare proposal that contain state transitions. Should we do that only if a specific logging level is enabled?

md5.update(&request.encode_to_vec());
Copy link
Member

Choose a reason for hiding this comment

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

BTW, BLAKE3 much faster than md5 (which is not using in crypto world at all)

let request_id = uuid::Uuid::from_bytes(md5.result())
.as_hyphenated()
.to_string();

let endpoint = abci_method_name(&value);
let request_id = uuid::Uuid::new_v4().to_string();
let endpoint = abci_method_name(value);

let span = match value {
Value::Info(_r) => tracing::span!(LEVEL, SPAN_NAME, endpoint, request_id),
Expand Down
2 changes: 1 addition & 1 deletion proto-compiler/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ pub fn abci_version<T: AsRef<Path>>(dir: T) -> String {
let contents = read_to_string(&file_path).expect("cannot read version/version.go");
use regex::Regex;

let re = Regex::new(r##"(?m)^\s+ABCISemVer\s*=\s*"([^"]+)"\s+*$"##).unwrap();
let re = Regex::new(r#"(?m)^\s+ABCISemVer\s*=\s*"([^"]+)"\s+*$"#).unwrap();
let captures = re
.captures(&contents)
.expect("cannot find ABCISemVer in version/version.go");
Expand Down