-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_dashboard: fix batcher failed txs panel #11570
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_dashboard: fix batcher failed txs panel #11570
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
matanl-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.
@matanl-starkware made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware).
crates/apollo_dashboard/src/panels/batcher.rs line 68 at r1 (raw file):
fn get_panel_rejection_reverted_ratio() -> Panel { let rejected_txs_expr = &REJECTED_TRANSACTIONS.get_name_with_filter();
I don't think this is better.
When looking at the entire counter (as opposed to an increase), it's almost impossible to spot recent changes.
IMO the formula should be:
increase(X) / increase(X +Y + Z)
Code quote:
&REJECTED_TRANSACTIONS.get_name_with_filter();
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 2 files reviewed, 1 unresolved discussion (waiting on @matanl-starkware).
crates/apollo_dashboard/src/panels/batcher.rs line 68 at r1 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
I don't think this is better.
When looking at the entire counter (as opposed to an increase), it's almost impossible to spot recent changes.
IMO the formula should be:
increase(X) / increase(X +Y + Z)
What does this value represent?
Why not increase(X / (X +Y + Z)) ?
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 2 files reviewed, 1 unresolved discussion (waiting on @matanl-starkware).
crates/apollo_dashboard/src/panels/batcher.rs line 68 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
What does this value represent?
Why not
increase(X / (X +Y + Z))?
... if the motivation is to track changes in X / (X +Y + Z) in an easier manner
7b7d36f to
7c4753b
Compare
matanl-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.
@matanl-starkware made 1 comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware).
crates/apollo_dashboard/src/panels/batcher.rs line 68 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
... if the motivation is to track changes in
X / (X +Y + Z)in an easier manner
According to Chat:
increase(X / (X+Y)) should not be ever used ("Apply increase() / rate() to the raw counters first, and only then compute ratios.")
Between these options, it recommends B ("This avoids subtle issues with label matching and aggregation.")
A. increase(X) / increase(X+Y)
B. increase(X) / (increase(X) + increase(Y))
matanl-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.
@matanl-starkware made 1 comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware).
crates/apollo_deployments/resources/app_configs/batcher_config.json line 27 at r3 (raw file):
"batcher_config.commitment_manager_config.results_channel_size": 1000, "batcher_config.commitment_manager_config.tasks_channel_size": 1000, "batcher_config.commitment_manager_config.wait_for_tasks_channel": true,
Seems unrelated to this PR
Code quote:
"batcher_config.commitment_manager_config.results_channel_size": 1000,
"batcher_config.commitment_manager_config.tasks_channel_size": 1000,
"batcher_config.commitment_manager_config.wait_for_tasks_channel": true,7c4753b to
b29cacb
Compare
b29cacb to
1bb6371
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 4 files reviewed, 2 unresolved discussions (waiting on @matanl-starkware).
crates/apollo_dashboard/src/panels/batcher.rs line 68 at r1 (raw file):
Previously, matanl-starkware (Matan Lior) wrote…
According to Chat:
increase(X / (X+Y)) should not be ever used ("Applyincrease()/rate()to the raw counters first, and only then compute ratios.")Between these options, it recommends B ("This avoids subtle issues with label matching and aggregation.")
A. increase(X) / increase(X+Y)
B. increase(X) / (increase(X) + increase(Y))
Done.
matanl-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.
@matanl-starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware).
-- commits line 2 at r5:
Consider adjusting the commit message according to actual change
Code quote:
fix batcher failed txs panel
matanl-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.
@matanl-starkware resolved 1 discussion.
Reviewable status: 2 of 4 files reviewed, all discussions resolved.
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 4 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware).
1557d95

No description provided.