-
Notifications
You must be signed in to change notification settings - Fork 37
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(starknet_sequencer_infra): adding metrics counters to local servers #4068
Conversation
d1b9983
to
7b65fd6
Compare
ddc8cd2
to
fe30eb5
Compare
7b65fd6
to
6ae9056
Compare
Benchmark movements: |
6ae9056
to
7e026c4
Compare
fe30eb5
to
128e619
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.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion
crates/starknet_sequencer_infra/src/metrics.rs
line 20 at r1 (raw file):
if counter.is_none() { warn!("Counter {} not found", name); }
Please use expect("Metric {} should be available")
instead of checking is_none()
.
(use the proper expect and formatting syntax 🙏 )
Code quote:
let counter = METRIC_COUNTERS_MAP.get(name);
if counter.is_none() {
warn!("Counter {} not found", name);
}
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.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @lev-starkware)
crates/starknet_sequencer_infra/src/metrics.rs
line 20 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Please use
expect("Metric {} should be available")
instead of checkingis_none()
.
(use the proper expect and formatting syntax 🙏 )
This will also require removing the Option
from the return type ^
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.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @lev-starkware)
crates/starknet_sequencer_infra/src/metrics.rs
line 20 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
This will also require removing the
Option
from the return type ^
OK I went over the code and I understand why you made it like this: you wanted the metrics map not to include mock metrics (which is great, the production code should not include mocks like that). But, this results in needing to hold the mock
field, which contradicts the above.
I'd like a solution that has neither: no mock metrics defined in the global level, and no mock fields or getters in the production code. Let's discuss this in person.
128e619
to
6985e91
Compare
7e026c4
to
28a4f5d
Compare
Benchmark movements: full_committer_flow performance improved 😺 |
28a4f5d
to
81ef3c3
Compare
6985e91
to
49b1d9c
Compare
81ef3c3
to
fe6b426
Compare
49b1d9c
to
1dc2de8
Compare
fe6b426
to
895f537
Compare
1dc2de8
to
61e3176
Compare
Benchmark movements: |
61e3176
to
93de4a4
Compare
895f537
to
5a0355b
Compare
Benchmark movements: full_committer_flow performance improved 😺 |
5a0355b
to
51f9518
Compare
Benchmark movements: full_committer_flow performance improved 😺 |
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.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion
crates/starknet_sequencer_infra/src/metrics.rs
line 8 at r2 (raw file):
/// A struct to contain all metrics for the a server/component. pub struct InfraMetrics { mock: bool,
Please remove the draft parts from this pr, e.g., mock: bool
Code quote:
mock: bool,
commit-id:4738c786
51f9518
to
cb60f70
Compare
Stack: