Conversation
PR SummaryMedium Risk Overview Deploy defaults shift to Ubuntu AMD64, larger gp3 volumes with configurable IOPS/throughput, and a longer readiness timeout. list, forward, and teardown understand per-validator hosts and print/use the Grafana URL. Adds an Autobahn E2E Grafana dashboard and Prometheus scrape wiring (local monitoring compose, AWS load host, monitornode scripts). Localnode Reviewed by Cursor Bugbot for commit 20ee39f. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4167 +/- ##
==========================================
- Coverage 66.69% 65.60% -1.09%
==========================================
Files 2202 2083 -119
Lines 169510 158064 -11446
==========================================
- Hits 113047 103691 -9356
+ Misses 56322 54232 -2090
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds an Autobahn e2e Grafana dashboard, main-loop phase metrics, and larger x86 AWS defaults. The metric names, labels and datasource uid all line up with the existing monitornode conventions, but the AWS deploy now opens Grafana to the whole internet with hardcoded admin/admin, which also makes the cluster's "private" EVM JSON-RPC reachable through Grafana's datasource proxy.
Findings: 1 blocking | 2 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ed51e2b. Configure here.
Bind Grafana to the SSH CIDR by default, ship the merged peer list with genesis, and record main-loop phases on the existing sei_chain meter. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
yzang2019
changed the title
There was a problem hiding this comment.
The three findings from the previous review are genuinely fixed: Grafana ingress now defaults to the SSH CIDR with an opt-in --grafana-cidr, the second global MeterProvider is no longer installed during storage-manager setup, and phase transitions are serialized behind gigametrics.SetPhase. Two non-blocking items remain — SetupPrometheus is now dead code that would re-break the metric namespace if anyone called it, and one README bullet still describes the old wide-open Grafana rule.
Findings: 0 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| ) | ||
|
|
||
| // SetupPrometheus installs a Prometheus MeterProvider on the default registerer. | ||
| func SetupPrometheus() error { |
There was a problem hiding this comment.
[suggestion] SetupPrometheus has no production caller — the only references in the tree are TestSetupPrometheusIsIdempotent and TestSetPhaseIsSafeForConcurrentCallers in this package's own test file. It is dead code whose one effect is to call otel.SetMeterProvider with a provider that has no namespace and no chain_id, i.e. exactly the state the previous round removed from openEVMOnlyStorageManager.
That matters because the new dashboard queries sei_chain_autobahn_main_loop_phase_duration_seconds_total, a name that only exists while the app-level provider from utilmetrics.SetupOtelMetricsProvider (app/app.go:511) is the global one. A future caller wiring up SetupPrometheus would silently move every lazily-created instrument off that provider and blank the panels, with nothing here saying so. The idempotency test reads as coverage for a function that is actually unreachable.
Deleting it (along with the otelprometheus/sdkmetric/prometheus imports) leaves SetPhase as the package's only entry point. MainLoop() at line 52 is then only reachable from SetPhase and the test, so it can be unexported too — which also removes the last handle through which a caller could mutate the shared timer outside loopMu.
| compile, and initialize in parallel. The load instance is brought up | ||
| afterward with Prometheus and Grafana; `sei-load` is left for you to | ||
| start. The security group admits SSH from the caller, Grafana (`:3000`) | ||
| from the internet, and all TCP between the five instances. |
There was a problem hiding this comment.
[suggestion] This bullet still describes the pre-fix behavior: Grafana is no longer opened to the internet. resolveGrafanaCIDR (cmd/autobahn-e2e/aws.go) defaults the :3000 rule to sshCIDR, and line 177 of this same file already says so correctly ("admits SSH and Grafana from the public IP detected at deployment time"). Suggest "admits SSH and Grafana (:3000) from the caller, and all TCP between the five instances", leaving the --grafana-cidr opt-in described where it already is.
Superseded: latest AI review found no blocking issues.
## Describe your changes and provide context Port of #4167 onto `giga-1`. Adds an Autobahn E2E Grafana dashboard, main-loop phase metrics, and an AWS `--topology` flag. **Dashboard.** Local and AWS clusters provision an **Autobahn E2E** Grafana board that shows executed TPS, blocks/sec, produce-to-execute finalize time, and the execute-goroutine split across consensus wait, EVM execution, and storage. Those last panels read `sei_chain_autobahn_main_loop_phase_duration_seconds_total`, recorded from the Autobahn execute path. **AWS topology.** `autobahn-e2e deploy --target aws` takes `--topology`: - `distributed` (default): five EC2 hosts — one validator per instance, plus a load/monitoring host. Validators clone, compile, and initialize in parallel. Prometheus and Grafana come up on the load box; `sei-load` is left for you to start. - `colocated`: one EC2 host running the same four Docker validators used locally, plus monitoring. The local Docker 4-in-1 path is unchanged. EVM JSON-RPC stays private and is reached through `forward`. **Defaults and access.** AWS deploy uses Ubuntu 24.04 AMD64, `r7i.12xlarge`, and a 40m readiness timeout. SSH and Grafana (`:3000`) default to the caller's public IP. Use `--grafana-cidr` to widen Grafana (for example `0.0.0.0/0`). Login remains `admin` / `admin` on these temporary test hosts. This branch also includes the #4167 review fixes: genesis ships the merged `persistent_peers.txt`, sibling `run-instances` calls no longer cancel each other, and phase instruments attach to the existing `sei_chain` meter instead of installing a second MeterProvider. ## Testing performed to validate your change - [x] `go test ./cmd/autobahn-e2e/ ./giga/metrics/` - [ ] `./autobahn-e2e deploy --target aws --topology distributed` — Grafana reachable from the SSH CIDR only; validators form a mesh - [ ] `./autobahn-e2e deploy --target aws --topology colocated` — four Docker validators on one instance - [ ] Local `./autobahn-e2e deploy --target local` still starts the 4-in-1 Docker cluster - [ ] Open Grafana **Autobahn E2E** and confirm the phase pie/stacked line populate after traffic --------- Co-authored-by: Cursor <cursoragent@cursor.com>
|
Is this the same as the one we are using on giga-1? |
3 participants
Describe your changes and provide context
This PR added monitoring dashboard for the autobahn-e2e benchmark tool. It also added a flag to allow deploying to AWS in colocated mode or distributed mode.
Testing performed to validate your change