Skip to content

Conversation

@victorkstarkware
Copy link
Contributor

No description provided.

@github-actions
Copy link

Artifacts upload workflows:

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

@Itay-Tsabary-Starkware reviewed 5 files and made 7 comments.
Reviewable status: 5 of 11 files reviewed, 7 unresolved discussions (waiting on @victorkstarkware).


Cargo.toml line 277 at r1 (raw file):

itertools = "0.12.1"
json-patch = "4.1.0"
jsonrpsee = "0.24"

Can you please add a patch version as well?

Code quote:

"0.24"

crates/apollo_rpc/src/middleware.rs line 2 at r1 (raw file):

use jsonrpsee::core::http_helpers::read_body;
use jsonrpsee::server::HttpRequest as Request;

Can we avoid this?

Code quote:

 as Request;

crates/apollo_rpc/src/rpc_metrics/mod.rs line 76 at r1 (raw file):

) -> RpcServiceBuilder<tower::layer::util::Stack<MetricLogger, tower::layer::util::Identity>> {
    RpcServiceBuilder::new().layer(MetricLogger::new(methods))
}

Do we need this wrapper function? Can we just call the inner function directly?

Code quote:

pub(crate) fn metric_logger_new(
    methods: &Methods,
) -> RpcServiceBuilder<tower::layer::util::Stack<MetricLogger, tower::layer::util::Identity>> {
    RpcServiceBuilder::new().layer(MetricLogger::new(methods))
}

crates/apollo_rpc/src/rpc_metrics/mod.rs line 88 at r1 (raw file):

/// A middleware service that logs metrics for each RPC call.
#[derive(Clone)]
pub struct MetricLoggerService<S> {

Can this be pub(crate)?

Code quote:

pub

crates/apollo_rpc/src/rpc_metrics/mod.rs line 111 at r1 (raw file):

/// Response future that records metrics when the response is ready.
pub struct MetricResponseFuture<F> {

Ditto

Code quote:

pub

crates/apollo_rpc/src/rpc_metrics/mod.rs line 77 at r1 (raw file):

    }

    #[cfg_attr(coverage_nightly, coverage_attribute)]

Why is this deleted? What does this do?

Code quote:

#[cfg_attr(coverage_nightly, coverage_attribute)]

crates/apollo_rpc/src/rpc_metrics/rpc_metrics_test.rs line 44 at r1 (raw file):

// Ignored because server_metrics test is running in parallel and we are unable to install multiple
// recorders.

Can we keep this, and re-enable it later?
This is solvable.

Code quote:

// Ignored because server_metrics test is running in parallel and we are unable to install multiple
// recorders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants