Fix mcp client rebuild bug && support labels for experiement && add metrics#70
Fix mcp client rebuild bug && support labels for experiement && add metrics#70JacksonMei merged 7 commits intomainfrom
Conversation
Add comprehensive Prometheus metrics (requests, latency, instance ops, active instances gauge, uptime gauge) to api-service. Implement labels propagation through full chain: SDK -> api-service -> faas-api-service -> metaservice, enabling per-experiment/owner/app metric breakdown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Share request metrics with MCP data plane traffic on port 8081. Skip recording /health endpoint errors to avoid noise from proxy-less K8s liveness probes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Redesign SDK logging to use stdlib hierarchy under "aenv" namespace. Callers can redirect all SDK logs with one line: setup_logging(handler). Default colored stdout output preserved for standalone usage. Bump version to 0.1.5. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for labels on environment instances and adds extensive Prometheus metrics for monitoring, alongside a significant refactoring of the Python SDK's logging. While these features enhance observability, security concerns were identified regarding the logging of sensitive information (environment variables, tool arguments) at high-visibility levels (INFO/ERROR) and the use of unbounded user-supplied input as Prometheus label values, which poses a Denial of Service (DoS) risk due to high cardinality. The review also focuses on improving code clarity and reducing repetition in the new metrics implementation and cleaning up the refactored logging module. It is recommended to sanitize logs, strictly control or aggregate metric labels derived from untrusted input, and refactor repetitive metric reporting for better code clarity.
| func (ctrl *EnvInstanceController) CreateEnvInstance(c *gin.Context) { | ||
| var req CreateEnvInstanceRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| metrics.InstanceOpsTotal.WithLabelValues("create", req.EnvName, "error").Inc() |
There was a problem hiding this comment.
The application uses user-supplied input (req.EnvName and id from URL parameters) as label values for Prometheus metrics (InstanceOpsTotal). This poses a significant risk of Denial of Service (DoS) due to high cardinality if an attacker provides a large number of unique values. Beyond this security concern, the current implementation also has repetitive calls to metrics.InstanceOpsTotal. Consider refactoring to use a defer statement to centralize metrics reporting, which can improve code clarity and potentially aid in sanitizing or aggregating label values.
| func (ctrl *EnvServiceController) CreateEnvService(c *gin.Context) { | ||
| var req CreateEnvServiceRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| metrics.ServiceOpsTotal.WithLabelValues("create", req.EnvName, "error").Inc() |
There was a problem hiding this comment.
The env_service controller uses user-supplied input (req.EnvName and id) as label values for ServiceOpsTotal metrics. This poses a significant risk of Denial of Service (DoS) due to high cardinality if an attacker provides many unique values for these fields. Similar to env_instance.go, the metrics reporting calls are repetitive. Consider using a defer statement to centralize the logic, which can improve code clarity and help manage label cardinality.
|
|
||
| logger.info( | ||
| f"Creating environment instance: {name}, datasource: {datasource}, ttl: {ttl}, environment_variables: {environment_variables}, arguments: {arguments}, owner: {owner}, url: {self.base_url}" | ||
| f"Creating environment instance: {name}, datasource: {datasource}, ttl: {ttl}, environment_variables: {environment_variables}, arguments: {arguments}, owner: {owner}, labels: {labels}, url: {self.base_url}" |
There was a problem hiding this comment.
The create_env_instance method logs the entire environment_variables and labels dictionaries at the INFO level. These dictionaries are highly likely to contain sensitive information such as API keys, passwords, or Personally Identifiable Information (PII) required for the environment setup. Logging this information can lead to sensitive data leakage in log files.
| logger.error( | ||
| f"{self._log_prefix()} Function '{function_url}' execution http request failed: {error_msg} | " | ||
| _log = logger.debug if quiet else logger.error | ||
| _log( |
There was a problem hiding this comment.
The _call_function and call_tool methods log the arguments passed to them at INFO or ERROR levels. These arguments could contain sensitive data intended for the tools or functions being executed. Logging them directly poses a risk of sensitive information disclosure.
| logger.error( | ||
| f"{self._log_prefix()} Function '{function_url}' execution failed: {str(e)} | " | ||
| _log = logger.debug if quiet else logger.error | ||
| _log( |
There was a problem hiding this comment.
The _call_function and call_tool methods log the arguments passed to them at INFO or ERROR levels. These arguments could contain sensitive data intended for the tools or functions being executed. Logging them directly poses a risk of sensitive information disclosure.
|
|
||
| logger.info( | ||
| f"{self._log_prefix()} Executing tool: {actual_tool_name} in environment {self.env_name}" | ||
| f"{self._log_prefix()} Executing tool: {actual_tool_name} in environment {self.env_name}, arguments={arguments}" |
There was a problem hiding this comment.
The _call_function and call_tool methods log the arguments passed to them at INFO or ERROR levels. These arguments could contain sensitive data intended for the tools or functions being executed. Logging them directly poses a risk of sensitive information disclosure.
| @@ -664,15 +670,17 @@ async def call_tool( | |||
|
|
|||
| except Exception as e: | |||
| logger.error( | |||
There was a problem hiding this comment.
The _call_function and call_tool methods log the arguments passed to them at INFO or ERROR levels. These arguments could contain sensitive data intended for the tools or functions being executed. Logging them directly poses a risk of sensitive information disclosure.
| app := inst.Labels["app"] | ||
|
|
||
| // Increment active instance count | ||
| ActiveInstances.WithLabelValues(env, experiment, owner, app).Inc() |
There was a problem hiding this comment.
The metrics collector extracts labels (env, experiment, owner, app) from instance labels, which are originally provided by users. These are then used as label values for ActiveInstances and InstanceUptimeSeconds gauges. This allows an attacker to inject arbitrary labels and cause high cardinality in the metrics system, potentially leading to a Denial of Service (DoS).
| metrics.RequestsTotal.WithLabelValues(method, endpoint, status).Inc() | ||
| metrics.RequestDurationMs.WithLabelValues(method, endpoint, status).Observe(durationMs) |
There was a problem hiding this comment.
The MetricsMiddleware uses the request path as a label value for RequestsTotal and RequestDurationMs metrics when c.FullPath() is empty. An attacker can send requests with many unique paths (e.g., random strings) to inflate the number of time series in Prometheus, leading to high cardinality and potential Denial of Service (DoS).
| name: Optional[str] = None, | ||
| type_: Optional[Literal["plain", "benchmark", "colored", "system"]] = None, |
There was a problem hiding this comment.
The type_ parameter in the getLogger function signature is no longer used after the logging refactoring. It appears to be a leftover from the previous implementation. To improve code clarity and maintainability, it should be removed.
name: Optional[str] = NonePreviously, each call_tool() and list_tools() used `async with client:` which created and destroyed a full MCP session (initialize → tools/call → DELETE) on every invocation. This wasted ~1.5s per call in session overhead and could cause 60s timeouts under load when re-initialization blocked. Add _ensure_mcp_session() that lazily enters the Client async context on first use and keeps the session alive for all subsequent calls. The session is only torn down in release(). Includes lock protection for concurrent access and automatic reconnection if the session is lost. Measured improvement: 10× echo calls from ~16s → ~5s total, subsequent calls from ~1.5s → ~0.6s each. Eliminates 60s timeout risk entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JacksonMei
changed the title
Remove mockEnvInstanceService and mockBackendClient that were defined but never used, causing golangci-lint unused errors in CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
No description provided.