-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_infra: cross component tracing lite #11415
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
apollo_infra: cross component tracing lite #11415
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
8bb3d9e to
b5b9bcc
Compare
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @victorkstarkware).
crates/apollo_infra/src/component_client/local_component_client.rs line 53 at r1 (raw file):
let (res_tx, mut res_rx) = channel::<Response>(1); let request_id = RequestId::generate(); info!(
debug at most, probably trace
Code quote:
info
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @victorkstarkware).
crates/apollo_infra/src/component_client/remote_component_client.rs line 260 at r1 (raw file):
max_attempts = max_attempts, "Sending remote request" );
Do we need a new request id per every attempt? Or could we place it before the loop?
Code quote:
let request_id = RequestId::generate();
info!(
request_id = %request_id,
request = %log_message,
attempt = attempt,
max_attempts = max_attempts,
"Sending remote request"
);
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @victorkstarkware).
crates/apollo_infra/src/component_client/local_component_client.rs line 53 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
debugat most, probablytrace
(Throughout)
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions (waiting on @victorkstarkware).
crates/apollo_infra/src/component_definitions.rs line 102 at r1 (raw file):
impl RequestIdGenerator for RequestId { fn generate() -> RequestId { rand::random::<u64>()
Please avoid fully-qualified fn calls; use a use rand::random at the top of the module and then just call random here
Code quote:
rand::random
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 4 unresolved discussions (waiting on @victorkstarkware).
crates/apollo_infra/src/component_definitions.rs line 98 at r1 (raw file):
pub trait RequestIdGenerator { fn generate() -> RequestId; }
Why do we need this trait def? Can the fn just be part of a vanilla impl block?
Code quote:
pub trait RequestIdGenerator {
fn generate() -> RequestId;
}
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @victorkstarkware).
crates/apollo_infra/Cargo.toml line 23 at r1 (raw file):
hyper = { workspace = true, features = ["client", "http2", "server", "tcp"] } metrics-exporter-prometheus.workspace = true rand.workspace = true
Is there an std equivalent for this?
Code quote:
rand.workspace = trueb5b9bcc to
cccaa20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victorkstarkware made 5 comments.
Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @Itay-Tsabary-Starkware).
crates/apollo_infra/Cargo.toml line 23 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Is there an
stdequivalent for this?
std::random is considered unstable. compiler points to this issue
crates/apollo_infra/src/component_definitions.rs line 98 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Why do we need this trait def? Can the fn just be part of a vanilla
implblock?
Done.
crates/apollo_infra/src/component_definitions.rs line 102 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Please avoid fully-qualified fn calls; use a
use rand::randomat the top of the module and then just callrandomhere
Done.
crates/apollo_infra/src/component_client/local_component_client.rs line 53 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
(Throughout)
Done.
crates/apollo_infra/src/component_client/remote_component_client.rs line 260 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Do we need a new request id per every attempt? Or could we place it before the
loop?
Done.
victorkstarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victorkstarkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @Itay-Tsabary-Starkware).
crates/apollo_infra/Cargo.toml line 23 at r1 (raw file):
Previously, victorkstarkware wrote…
random is considered unstable. compiler points to this issue
as in std::random
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware made 2 comments and resolved 5 discussions.
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @victorkstarkware).
crates/apollo_infra/src/component_server/remote_component_server.rs line 140 at r2 (raw file):
// Wrap the send operation in a tokio::spawn as it is NOT a cancel-safe operation. // Even if the current task is cancelled, the inner task will continue to run. //
Please remove
Code quote:
//crates/apollo_infra/src/component_server/local_component_server.rs line 398 at r2 (raw file):
component = component_name, "Receiving local request" );
I'm not familiar with this syntax. What does it do?
Code quote:
trace!(
request_id = %request_id,
request = request_label,
component = component_name,
"Receiving local request"
);cccaa20 to
5f9646b
Compare
victorkstarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victorkstarkware made 2 comments.
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware).
crates/apollo_infra/src/component_server/local_component_server.rs line 398 at r2 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
I'm not familiar with this syntax. What does it do?
it simply adds more items to the JSON, similar to "timestamp". here is an example:
Code snippet:
{"timestamp":"2026-01-05T12:10:21.413Z","level":"INFO","message":"Receiving local request","request_id":"2748595246222769631","request":"get_transactions","component":"MempoolCommunicationWrapper","filename":"crates/apollo_infra/src/component_server/local_component_server.rs","line_number":393}crates/apollo_infra/src/component_server/remote_component_server.rs line 140 at r2 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Please remove
Done.
25050c9 to
5d8dd87
Compare
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions (waiting on @victorkstarkware).
crates/apollo_infra/src/component_client/remote_component_client.rs line 258 at r4 (raw file):
trace!("Request {log_message} attempt {attempt} of {max_attempts}"); let http_request = self.construct_http_request(serialized_request_bytes.clone(), request_id.clone());
Can we pass a reference here instead of cloning? It seems the passed value is used to generate a string later on, and if so, a ref should suffice.
Code quote:
request_id.clone()
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Itay-Tsabary-Starkware resolved 2 discussions.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @victorkstarkware).
5d8dd87 to
3071b32
Compare
victorkstarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victorkstarkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware).
crates/apollo_infra/src/component_client/remote_component_client.rs line 258 at r4 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Can we pass a reference here instead of cloning? It seems the passed value is used to generate a string later on, and if so, a ref should suffice.
good point
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this 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 8 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @victorkstarkware).
3071b32 to
a1763bf
Compare
victorkstarkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@victorkstarkware reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @victorkstarkware).
c8a8fc8

No description provided.