Repository navigation
mcpx doctor: slow-remote check from measured call stats (#1936) - #2597
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 38 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5125d38c4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !strings.EqualFold(srv.Name, name) { | ||
| continue |
There was a problem hiding this comment.
For remotes added through mcpx registry add, this comparison rejects the corresponding registry entry: registry.ToInstall derives the config key using registry.Namespace, so io.github.user/weather is configured as weather, which is also the name recorded in call statistics. Searching for weather can return the correct entry, but it will never equal the full registry name here. Consequently, normally registry-installed remotes receive no local-package recommendation even when one exists. Resolve the registry identity while accounting for namespace ambiguity, and retain its full name for the suggested command.
Useful? React with 👍 / 👎.
| fix = fmt.Sprintf("host it locally instead: `mcpx registry add %s --write` installs %s; "+ | ||
| "the numbers above are what the round trip costs today", s.Name, pkg) |
There was a problem hiding this comment.
When a matching registry entry offers both a remote endpoint and a local package, this command does not install the advertised package. The --local flag's help in internal/cli/registry.go says “prefer an installable package over a remote”; without it, the add handler calls ToInstall(false), whose implementation selects an SSE or streamable-HTTP remote before considering packages. Include --local in the recommendation so following the local-hosting advice does not configure another remote endpoint.
Useful? React with 👍 / 👎.
| rows, err := st.Calls(logstore.Query{}) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| stats := aggregateRemoteStats(rows) |
There was a problem hiding this comment.
When two project configurations use the same server name, this query combines their histories and attributes the result to the current project's URL. daemon.Paths.ForConfig changes only the socket and info paths, leaving the default State/logs directory shared; the log store ingests all JSONL files there, and Calls groups records solely by server name and tool. Thus 50 slow calls to a local weather server in one project can produce a remote-latency warning for an entirely uncalled remote weather server in another project, bypassing the intended volume floor and reporting unrelated measurements. Associate the records with the relevant configuration/server identity before applying the thresholds.
Useful? React with 👍 / 👎.
| servers, err := cfg.ResolveAll() | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
For a supported configuration such as "pool":{"sharing":"SHARED"}, this helper silently skips all remote-latency checks. The settings layer normalizes enum values case-insensitively, and both the daemon and the existing doctor configuration check call config.ApplyPoolSettings before resolving servers, so that configuration otherwise works. This helper instead reloads the raw configuration and passes the unnormalized value to ResolveAll, which rejects it and triggers the early return. Apply the resolved pool settings here as well, or reuse the configuration already resolved by CmdDoctor.
Useful? React with 👍 / 👎.
The #1936 best item (queue task 3233): a doctor check built from evidence mcpx already collects.
What changed
internal/cli/doctor_remote.go, wired intoCmdDoctor:logstore.Calls) into per-server figures: calls, errors, error rate, and the slowest tool's p95.remote latencywarning naming the server, its URL, and its measured numbers./v1/registry/search) whether an entry under that exact name carries an installable package; if so the fix recommends local hosting with the package (mcpx registry add <name> --writeinstalls …). Registry failure costs the recommendation, never the report.doctor_remote_test.go) cover the four named cases — slow remote with a local package (recommendation fires, numbers named), slow remote without one (report only), local command server (never flagged, even with terrible stats), low-volume remote (silent below the floor) — plus the per-tool→per-server aggregation.Tests (laptop)
go test ./internal/cli/ -run 'TestSlowRemote|TestLocalCommand|TestLowVolume|TestAggregateRemote'— all pass. Fullgo test ./...runs in CI.Need help on this PR? Tag
@codesmith-botwith what you need.