refactor logger tail http requests to api_client #422
Conversation
WalkthroughRefactors logger tail command to use a new authenticated MCPAppClient.get_app_logs method. Adds LogEntry and GetAppLogsResponse models with unified accessors. Implements parameter mapping for order_by/order, updates response handling, and adds targeted error handling for auth and HTTP status codes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI logger tail
participant Auth as setup_authenticated_client
participant Client as MCPAppClient
participant API as /mcp_app/get_app_logs
User->>CLI: mcp logger tail [flags]
CLI->>Auth: initialize authenticated client
Auth-->>CLI: MCPAppClient
CLI->>Client: get_app_logs(app_id/app_configuration_id, since, limit, order_by, order)
Client->>API: POST payload
API-->>Client: GetAppLogsResponse
Client-->>CLI: response.log_entries_list
CLI-->>User: render log entries
rect rgba(230,240,255,0.6)
note over Client,API: New: centralized API call and response models
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
jtcorbett
force-pushed
the
09-05-refactor_logger_tail_http_requests_to_api_client
branch
from
ac3f45b to
92622d0
Compare
There was a problem hiding this comment.
looks like @rholinshead is already reviewing, but just requesting changes for the requirements.txt
jtcorbett
dismissed
saqadri’s stale review
Ryan already reviewed. Addressed feedback
jtcorbett
force-pushed
the
09-05-add_cloud_servers
branch
from
f6e5d35 to
7dc6cf6
Compare
jtcorbett
force-pushed
the
09-05-refactor_logger_tail_http_requests_to_api_client
branch
from
8a35062 to
273b208
Compare
Merge activity
|
jtcorbett
changed the base branch from
09-05-add_cloud_servers
to
graphite-base/422
jtcorbett
force-pushed
the
09-05-refactor_logger_tail_http_requests_to_api_client
branch
from
273b208 to
1546ad1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (1)
183-216: Regression: server URL isn’t handled in non-follow mode.
_fetch_logsnow ignoresserver_url, so passing a server URL (allowed by CLI help) yields aValueErrorfrom the client. Resolve the URL to an app/config ID before callingget_app_logs.client = setup_authenticated_client() @@ # Map order parameter from CLI to API format order_param = None if asc: order_param = "LOG_ORDER_ASC" elif desc: order_param = "LOG_ORDER_DESC" + # Resolve server_url to an ID if needed (non-follow mode accepts server URL) + if not app_id and not config_id and server_url: + try: + target = await client.get_app_or_config(server_url) + app_id = getattr(target, "appId", None) + config_id = getattr(target, "appConfigurationId", None) + except httpx.HTTPStatusError as e: + if e.response.status_code == 404: + raise CLIError("App or configuration not found") + raise + with Progress( SpinnerColumn(), TextColumn("[progress.description]{task.description}"), console=console, transient=True, ) as progress:
♻️ Duplicate comments (1)
src/mcp_agent/cli/mcp_app/api_client.py (1)
118-127: Use a single, canonical response field (logEntries) and default factories.Past comment notes only
logEntriesis valid. Keep one source of truth and avoid mutable defaults.class GetAppLogsResponse(BaseModel): """Response from get_app_logs API endpoint.""" - logEntries: Optional[List[LogEntry]] = [] - log_entries: Optional[List[LogEntry]] = [] + logEntries: List[LogEntry] = Field(default_factory=list) @property def log_entries_list(self) -> List[LogEntry]: """Get log entries regardless of field name format.""" - return self.logEntries or self.log_entries or [] + return self.logEntries
🧹 Nitpick comments (3)
src/mcp_agent/cli/mcp_app/api_client.py (1)
650-664: Align request payload casing with the rest of the client (camelCase).Other methods use camelCase at the API boundary. For consistency and to reduce backend ambiguity, switch to camelCase keys.
# Prepare request payload payload = {} if app_id: - payload["app_id"] = app_id + payload["appId"] = app_id if app_configuration_id: - payload["app_configuration_id"] = app_configuration_id + payload["appConfigurationId"] = app_configuration_id if since: payload["since"] = since if limit: payload["limit"] = limit if order_by: - payload["order_by"] = order_by + payload["orderBy"] = order_by if order: payload["order"] = orderIf the server indeed accepts both casings (as mentioned in earlier discussion), this is a no-op behaviorally but improves consistency. If not, keep as-is.
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (2)
186-199: Mapping looks right; consider centralizing enum constants.Hardcoding
"LOG_ORDER_BY_TIMESTAMP","LOG_ORDER_BY_LEVEL","LOG_ORDER_ASC","LOG_ORDER_DESC"is brittle. Export enums from the API client to prevent typos drifting across files.I can add
Enumtypes inapi_client.pyand reuse them here to remove magic strings.
220-229: Catch client-sideValueErrorfor nicer CLI UX.
get_app_logsraisesValueErrorfor invalid/missing IDs. Convert toCLIErrorwith a friendly message.- except UnauthenticatedError: + except UnauthenticatedError: raise CLIError("Authentication failed. Try running 'mcp-agent login'") + except ValueError as e: + # Input validation errors from API client (e.g., missing IDs or bad prefixes) + raise CLIError(str(e))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py(3 hunks)src/mcp_agent/cli/mcp_app/api_client.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcp_agent/cli/mcp_app/api_client.py (2)
src/mcp_agent/cli/core/api_client.py (1)
post(75-87)tests/executor/temporal/test_execution_id_and_interceptor.py (1)
json(62-63)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (3)
src/mcp_agent/cli/cloud/commands/servers/utils.py (1)
setup_authenticated_client(18-36)src/mcp_agent/cli/core/api_client.py (1)
UnauthenticatedError(9-12)src/mcp_agent/cli/mcp_app/api_client.py (2)
get_app_logs(612-669)log_entries_list(124-126)
🔇 Additional comments (2)
src/mcp_agent/cli/cloud/commands/logger/tail/main.py (2)
20-22: Good move centralizing auth.Switching to
setup_authenticated_clientand catchingUnauthenticatedErrormatches the new API client flow.
218-219: LGTM: normalize to dicts for display.Using
model_dump()keeps display code unchanged while benefiting from typed models.
| class LogEntry(BaseModel): | ||
| """Represents a single log entry.""" | ||
| timestamp: Optional[str] = None | ||
| level: Optional[str] = None | ||
| message: Optional[str] = None | ||
| # Allow additional fields that might be present | ||
|
|
||
| class Config: | ||
| extra = "allow" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pydantic v2 config isn’t applied; switch to model_config = ConfigDict(extra="allow").
class Config is ignored in Pydantic v2. Your intent is to keep unknown fields on LogEntry; use ConfigDict instead.
Apply within this block:
class LogEntry(BaseModel):
"""Represents a single log entry."""
timestamp: Optional[str] = None
level: Optional[str] = None
message: Optional[str] = None
- # Allow additional fields that might be present
-
- class Config:
- extra = "allow"
+ # Allow additional fields that might be present
+ model_config = ConfigDict(extra="allow")Also update imports (outside this hunk):
from pydantic import BaseModel, ConfigDict, Field🤖 Prompt for AI Agents
In src/mcp_agent/cli/mcp_app/api_client.py around lines 107 to 116, the Pydantic
v2 configuration using class Config is ignored; replace it with model_config =
ConfigDict(extra="allow") on the LogEntry model so unknown fields are preserved,
and update the imports at the top to include ConfigDict (i.e., from pydantic
import BaseModel, ConfigDict, Field) so the new config reference resolves.
TL;DR
Refactored the log fetching functionality to use the API client and added a new
get_app_logsmethod to theMCPAppClientclass.What changed?
mcp_basic_agentexample to use the published package version 0.1.16 instead of a local file reference_fetch_logsfunction in the logger tail command to use the API client instead of making direct HTTP requestsLogEntryandGetAppLogsResponseto properly type and handle log dataget_app_logsmethod in theMCPAppClientclass to fetch logs from the APIHow to test?
mcp-agent cloud logger tail <app-id>to verify logs are fetched correctly--since,--limit,--order-by, and--asc/--descflagsWhy make this change?
This change improves code maintainability by centralizing API interactions in the client class rather than implementing them directly in command handlers. It also provides proper typing for log entries and standardizes the approach to API requests, making the codebase more consistent and easier to maintain.
Summary by CodeRabbit
New Features
Bug Fixes