-
Notifications
You must be signed in to change notification settings - Fork 35
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
base: spr/main/34419b89
Are you sure you want to change the base?
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.
commit-id:4738c786
128e619
to
6985e91
Compare
7e026c4
to
28a4f5d
Compare
Benchmark movements: full_committer_flow performance improved 😺 |
Stack: