-
Notifications
You must be signed in to change notification settings - Fork 772
[rfc] fuzzy matching to exporter schema #550
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
Conversation
WalkthroughAdds type inference for OpenTelemetry exporter configs lacking explicit "type". Introduces OpenTelemetrySettings._guess_exporter_type and updates _coerce_exporters_schema to normalize list-based exporters, infer types (otlp/file) from keys, and raise errors when inference fails. Maintains legacy string and per-exporter field handling. Adds tests covering inference and error cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Config as OpenTelemetrySettings
participant Coerce as _coerce_exporters_schema
participant Guess as _guess_exporter_type
User->>Config: load(settings with exporters)
Config->>Coerce: normalize exporters
alt exporters is list
loop each entry
Coerce->>Coerce: if BaseModel -> to_dict
alt entry has explicit type
Coerce->>Coerce: validate and keep
else entry missing type
Coerce->>Guess: infer type by keys
alt type inferred (otlp/file)
Coerce->>Coerce: inject inferred type, keep fields
else cannot infer
Coerce-->>User: raise ValueError
end
end
end
else exporters is string (console/file/otlp)
Coerce->>Coerce: up-convert using legacy fields
end
Coerce-->>Config: normalized typed exporters
Config-->>User: finalized settings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/mcp_agent/config.py (2)
608-624
: Consider field-overlap edge cases and type precision.The inference logic checks OTLP keys before file keys, so an entry with both
endpoint
andpath
would be inferred as OTLP, potentially ignoring thepath
field. While this may not be a current issue, it could cause confusion if configurations inadvertently include mixed keys.Additionally, the return type
str | None
is less precise thanLiteral["otlp", "file"] | None
, which would provide better type safety.Consider these improvements:
- Add validation to reject entries with conflicting keys (e.g., both OTLP and file keys present).
- Refine the return type annotation:
-def _guess_exporter_type(entry: Dict[str, Any]) -> str | None: +def _guess_exporter_type(entry: Dict[str, Any]) -> Literal["otlp", "file"] | None:
- Document that console exporters cannot be inferred and must have an explicit
type
field.
639-666
: LGTM with minor suggestion!The normalization and type inference logic is well-implemented. It correctly handles BaseModel conversion, infers missing types, and provides clear error messages when inference fails.
One minor enhancement: The error message references "secrets overlays," which is helpful context from the PR description. However, since this validator runs for all configurations (not just overlays), consider making the message more general:
else: raise ValueError( "OpenTelemetry exporter entries must include a 'type'. " "Unable to infer exporter type from fields " f"{sorted(entry_dict.keys())}. " - "Ensure each exporter in secrets overlays sets `type`." + "Provide an explicit 'type' field or include recognizable " + "keys (e.g., 'endpoint' for OTLP, 'path' for file)." )tests/test_config_exporters.py (1)
106-148
: Excellent test coverage!The new tests comprehensively validate the type inference logic across the key scenarios:
- OTLP inference from endpoint and headers (both together and headers alone)
- File inference from path_settings
- Error handling for unrecognizable fields
The tests are well-structured, use existing assertion helpers, and align with the PR objectives of enabling fuzzy matching for exporter configurations.
Consider adding a test case that explicitly verifies console exporters require an explicit
type
field, since they cannot be inferred:def test_console_type_cannot_be_inferred(): """Console exporters require explicit type since they have no unique keys.""" # This should fail since console has no unique keys for inference with pytest.raises(ValueError, match="must include a 'type'"): OpenTelemetrySettings(exporters=[{}])This would document the intended behavior and prevent regression if the inference logic changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mcp_agent/config.py
(2 hunks)tests/test_config_exporters.py
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_config_exporters.py (1)
src/mcp_agent/config.py (2)
OpenTelemetrySettings
(576-767)OTLPExporterSettings
(560-563)
class ConsoleExporterSettings(OpenTelemetryExporterBase): | ||
type: Literal["console"] = "console" | ||
|
||
|
||
class FileExporterSettings(OpenTelemetryExporterBase): | ||
type: Literal["file"] = "file" | ||
path: str | None = None | ||
path_settings: TracePathSettings | None = None | ||
|
||
|
||
class OTLPExporterSettings(OpenTelemetryExporterBase): | ||
type: Literal["otlp"] = "otlp" | ||
endpoint: str | None = None | ||
headers: Dict[str, str] | None = None |
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.
i think the root cause of issue is here. when we have "type: console", for example:
- because "console" is the default value of type, when we deserialize settings object from base setting yaml with "ignore defaults", this ConsoleExporterSettings will be deserialzied to empty object {}.
- then, we will use the yaml merge to merge the secrets volume into this base settings object, Pydantic validator will get an {} empty and Pydantic will fail at validation because without "type", it cannot decide which type that is.
Closing this PR in favor of #551 |
Ensure OpenTelemetrySettings.exporters tolerate list items that drop the type, inferring it when possible and emitting a clear error otherwise.
Example:
base config (mcp_agent.config.yaml):
overlay (mcp_agent.secrets.yaml):
merged result (what we load):