refactor: otel settings have stable (de)serialization #551
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors OpenTelemetry exporter config to a single-key mapping/list schema (V3), adds pre/post validators to normalize legacy shapes, updates tracer parsing to accept payload-based exporter entries (console/file/otlp) with legacy fallbacks, and updates examples, schema, and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant YAML as User Config / Secrets
participant Settings as OpenTelemetrySettings
participant Tracer as Tracer Init
participant Factory as Exporter Factory
YAML->>Settings: load config (V1/V2/V3 shapes)
activate Settings
Settings->>Settings: _coerce_exporters_schema (before)\nnormalize legacy shapes -> V3 single-key maps
Settings->>Settings: _finalize_exporters (after)\nresolve legacy fields, build final exporters list
Settings-->>YAML: validated Settings (exporters normalized)
deactivate Settings
YAML->>Tracer: init tracer with Settings
activate Tracer
loop each exporter entry
Tracer->>Factory: parse entry -> (type, payload)
alt console
Factory-->>Tracer: build console exporter
else file
Factory->>Factory: validate/convert path_settings (TracePathSettings)
Factory-->>Tracer: build file exporter
else otlp
Factory->>Factory: resolve endpoint/headers (payload or legacy)
Factory-->>Tracer: build otlp exporter (or skip if no endpoint)
else unknown
Tracer-->>Tracer: log error, skip
end
end
Tracer-->>YAML: tracer provider initialized
deactivate Tracer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
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 |
alienzach
force-pushed
the
10-09-feat_v3_otel_settings
branch
from
272f20b to
f61ba40
Compare
| Dict[Literal["console"], ConsoleExporterSettings | Dict], | ||
| Dict[Literal["file"], FileExporterSettings | Dict], | ||
| Dict[Literal["otlp"], OTLPExporterSettings | Dict], |
There was a problem hiding this comment.
Why the | Dict?
| Dict[Literal["console"], ConsoleExporterSettings | Dict], | |
| Dict[Literal["file"], FileExporterSettings | Dict], | |
| Dict[Literal["otlp"], OTLPExporterSettings | Dict], | |
| Dict[Literal["console"], ConsoleExporterSettings], | |
| Dict[Literal["file"], FileExporterSettings], | |
| Dict[Literal["otlp"], OTLPExporterSettings], |
There was a problem hiding this comment.
when we validate the schema of concrete type from yaml, we don't know which concrete type it is yet. we have to use dict to allow yaml to be deserialized into BaseModel then create concrete model after we get the type from key.
There was a problem hiding this comment.
Hmmm, but does it cause an issue? Why was the previous case ok where we had it as Union[Literal["console", "file", "otlp"], OpenTelemetryExporterSettings] (no | Dict)
src/mcp_agent/config.py
Outdated
| """Sample rate for tracing (1.0 = sample everything)""" | ||
|
|
||
| # Deprecated: use exporters: [{ type: "otlp", ... }] | ||
| # Deprecated V1 field: use exporters list with V3 syntax instead |
There was a problem hiding this comment.
The V1 vs. V3 is quite arbitrary. Just describe what the expected syntax is
|
@saqadri PR is ready. But how do we want to approach the secret merging in this multi exporters schema? for example, here: https://github.com/lastmile-ai/mcp-agent/blob/5e99d4dc8294fa3b5f949e2938f8ce16ec620e6e/examples/tracing/langfuse/README.md#2-set-up-secrets-and-environment-variables we could use endpoint here to match the headers in secret and config, which mean the url have to be specified in both places as a key, also have to be unique? or maybe I am over thinking? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/tracing/tracer.py (1)
231-233: Fix copy/paste in OpenAI instrumentation error messageWrong package name; should reference the OpenAI instrumentation.
Apply:
- logger.error( - "OpenAI OTEL instrumentation not available. Please install opentelemetry-instrumentation-anthropic." - ) + logger.error( + "OpenAI OTEL instrumentation not available. Please install opentelemetry-instrumentation-openai." + )
🧹 Nitpick comments (4)
src/mcp_agent/tracing/tracer.py (1)
117-135: Optional: simplify to V3-only exporter parsingOpenTelemetrySettings._finalize_exporters normalizes to V3 single-key dicts. You can drop the legacy string-path in the tracer for simpler, stricter handling (config preserves back-compat).
Example:
- Assume exporter is a single-key dict and error if not.
- Removes isinstance(exporter, str) and unknown format branch.
tests/test_config_exporters.py (2)
4-4: Prefer importing ValidationError from pydantic for stabilityAvoid coupling tests to pydantic_core internals; import from pydantic.
Apply:
-from pydantic_core import ValidationError +from pydantic import ValidationError
360-364: Standardize on ValidationError for schema failuresThe model validators raise validation errors; asserting ValidationError is more consistent across cases.
Apply:
-def test_invalid_exporter_format_raises(): - """Test that invalid exporter formats raise ValueError.""" - with pytest.raises(ValueError, match="OpenTelemetry exporters must be strings"): - OpenTelemetrySettings(exporters=[{"foo": "bar", "baz": "qux"}]) # Multi-key dict +def test_invalid_exporter_format_raises(): + """Test that invalid exporter formats raise ValidationError.""" + with pytest.raises(ValidationError, match="OpenTelemetry exporters must be strings"): + OpenTelemetrySettings(exporters=[{"foo": "bar", "baz": "qux"}]) # Multi-key dictsrc/mcp_agent/config.py (1)
584-596: Docstring example could use YAML-accurate list syntaxTo avoid confusion, show list items with dashes for YAML.
Example:
- exporters:
- console: {}
- file:
path: "trace.jsonl"- otlp:
endpoint: "https://..."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
examples/tracing/agent/README.md(1 hunks)examples/tracing/agent/mcp_agent.config.yaml(1 hunks)examples/tracing/langfuse/README.md(1 hunks)examples/tracing/langfuse/mcp_agent.config.yaml(1 hunks)examples/tracing/llm/README.md(1 hunks)examples/tracing/llm/mcp_agent.config.yaml(1 hunks)examples/tracing/mcp/README.md(1 hunks)examples/tracing/mcp/mcp_agent.config.yaml(1 hunks)examples/tracing/temporal/README.md(1 hunks)examples/tracing/temporal/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_deep_orchestrator/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_intent_classifier/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_orchestrator_worker/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_parallel/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_router/mcp_agent.config.yaml(1 hunks)src/mcp_agent/config.py(4 hunks)src/mcp_agent/tracing/tracer.py(3 hunks)tests/test_config_exporters.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_config_exporters.py (1)
src/mcp_agent/config.py (4)
OpenTelemetrySettings(565-789)TracePathSettings(498-521)TraceOTLPSettings(524-535)Settings(868-981)
src/mcp_agent/tracing/tracer.py (2)
src/mcp_agent/config.py (2)
OpenTelemetrySettings(565-789)TracePathSettings(498-521)src/mcp_agent/logging/logger.py (1)
error(291-299)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: checks / format
- GitHub Check: update_release_draft
🔇 Additional comments (5)
src/mcp_agent/tracing/tracer.py (2)
166-179: LGTM: path_settings coercionGood normalization of dict to TracePathSettings to keep FileSpanExporter typed.
136-141: Ignore removal ofservice_nameargument ConsoleSpanExporter’s constructor supportsservice_name, so no change is needed.Likely an incorrect or invalid review comment.
tests/test_config_exporters.py (1)
216-231: Solid coverage of V3 dict-key schemaGood assertions on multi-exporter V3 usage and field presence.
src/mcp_agent/config.py (2)
610-675: Schema coercion looks robustNice handling of V1 strings, V2 type-discriminators, and V3 key-discriminators, plus BaseModel passthrough.
677-789: Finalization pipeline is clear and backward-compatibleGood use of typed model_validate, legacy fallbacks, and removal of legacy extras to keep dumps clean.
examples/tracing/agent/README.md
Outdated
| - console: {} | ||
| - file: {} |
There was a problem hiding this comment.
Maybe we set these to file and console respectively, which would have them be treated as strings
|
@alienzach I don't think we should do any sophisticated merge. The secrets one should be updated to match the schema, so you should update that readme and example to show the secrets as: In fact, if there are secrets to specify for the Otel exporters, that list item should just be specified in the secrets.yaml instead of config.yaml. Just validate that this case would work: config.yaml: secrets.yaml: Final config should include BOTH in the exporters list. |
examples/tracing/llm/README.md
Outdated
| - console: {} | ||
| - file: {} |
There was a problem hiding this comment.
For these empty cases, let's switch to the string literals...
console
file
There was a problem hiding this comment.
Some other comments for the overall PR:
-
Can we please get rid of comment references to "V1", "V2", "V3", etc... those are arbitrary since we don't have any versioning of the config.
-
Please regenerate the schema with "make schema"
-
Anywhere we have things like "file: {}", or "console: {}", let's replace with just "file" and "console" respectively, as the string literal will take effect.
Lastly just do some manual testing across the examples to ensure it all works. Thanks for the change!
…ring\n feat: test cases
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/tracing/llm/mcp_agent.config.yaml (1)
29-34: Adopt the V3 single-key mapping syntax for exporters.These entries are still V1-style scalars. To match the new schema (and enable secrets/config merging per exporter), please switch to the V3 shape, e.g.:
- exporters: - - console - - file + exporters: + - console: {} + - file: + path: "trace.jsonl"You can tweak the file configuration as needed, but each exporter should be represented as a one-key mapping.
tests/test_config_exporters.py (1)
473-499: Extract duplicateddeep_mergefunction to shared helper.The
deep_mergefunction is duplicated in two test functions. Consider extracting it to a module-level helper function to improve maintainability and reduce code duplication.Apply this diff to extract the shared helper:
+def _deep_merge_with_otel_concatenation(base: dict, update: dict, path: tuple = ()) -> dict: + """Recursively merge two dictionaries, preserving nested structures. + + Special handling for 'exporters' lists under 'otel' key: + - Concatenates lists instead of replacing them + - Allows combining exporters from config and secrets files + """ + merged = base.copy() + for key, value in update.items(): + current_path = path + (key,) + if ( + key in merged + and isinstance(merged[key], dict) + and isinstance(value, dict) + ): + merged[key] = _deep_merge_with_otel_concatenation(merged[key], value, current_path) + elif ( + key in merged + and isinstance(merged[key], list) + and isinstance(value, list) + and current_path == ("otel", "exporters") + ): + # Concatenate exporters lists from config and secrets + merged[key] = merged[key] + value + else: + merged[key] = value + return merged + + def test_merge_otel_exporters_from_config_and_secrets(): """Test that OTEL exporters from config.yaml and secrets.yaml are merged together.""" - from mcp_agent.config import get_settings # Simulate config.yaml with one OTLP exporter (public endpoint) config_data = { ... } # Simulate secrets.yaml with another OTLP exporter (secret endpoint) secrets_data = { ... } - # Manually perform deep merge as get_settings does internally - def deep_merge(base: dict, update: dict, path: tuple = ()) -> dict: - """Recursively merge two dictionaries, preserving nested structures. - - Special handling for 'exporters' lists under 'otel' key: - - Concatenates lists instead of replacing them - - Allows combining exporters from config and secrets files - """ - merged = base.copy() - for key, value in update.items(): - current_path = path + (key,) - if ( - key in merged - and isinstance(merged[key], dict) - and isinstance(value, dict) - ): - merged[key] = deep_merge(merged[key], value, current_path) - elif ( - key in merged - and isinstance(merged[key], list) - and isinstance(value, list) - and current_path == ("otel", "exporters") - ): - # Concatenate exporters lists from config and secrets - merged[key] = merged[key] + value - else: - merged[key] = value - return merged - - merged = deep_merge(config_data, secrets_data) + merged = _deep_merge_with_otel_concatenation(config_data, secrets_data) settings = Settings(**merged) ...Then update
test_merge_non_otel_lists_are_replaced_not_concatenatedsimilarly:def test_merge_non_otel_lists_are_replaced_not_concatenated(): """Test that non-OTEL lists are replaced, not concatenated (default behavior).""" - # Manually perform deep merge as get_settings does internally - def deep_merge(base: dict, update: dict, path: tuple = ()) -> dict: - ... - return merged - # Test with logger.transports (should be replaced, not concatenated) config_data = { ... } secrets_data = { ... } - merged = deep_merge(config_data, secrets_data) + merged = _deep_merge_with_otel_concatenation(config_data, secrets_data) ...Also applies to: 525-551
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
examples/tracing/agent/README.md(1 hunks)examples/tracing/agent/mcp_agent.config.yaml(1 hunks)examples/tracing/langfuse/README.md(1 hunks)examples/tracing/langfuse/mcp_agent.secrets.yaml.example(1 hunks)examples/tracing/llm/README.md(1 hunks)examples/tracing/llm/mcp_agent.config.yaml(1 hunks)examples/tracing/temporal/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_intent_classifier/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_orchestrator_worker/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_parallel/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_router/mcp_agent.config.yaml(1 hunks)src/mcp_agent/config.py(5 hunks)src/mcp_agent/tracing/tracer.py(3 hunks)tests/test_config_exporters.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml
- examples/workflows/workflow_orchestrator_worker/mcp_agent.config.yaml
- examples/tracing/agent/mcp_agent.config.yaml
- examples/workflows/workflow_parallel/mcp_agent.config.yaml
- examples/tracing/llm/README.md
- examples/workflows/workflow_router/mcp_agent.config.yaml
- examples/tracing/langfuse/README.md
🧰 Additional context used
🧬 Code graph analysis (3)
src/mcp_agent/tracing/tracer.py (1)
src/mcp_agent/config.py (2)
OpenTelemetrySettings(565-788)TracePathSettings(498-521)
tests/test_config_exporters.py (1)
src/mcp_agent/config.py (6)
OpenTelemetrySettings(565-788)TracePathSettings(498-521)TraceOTLPSettings(524-535)Settings(867-980)get_settings(1047-1161)deep_merge(1059-1085)
src/mcp_agent/config.py (1)
tests/test_config_exporters.py (2)
deep_merge(473-499)deep_merge(525-551)
🪛 GitHub Actions: Pull Request Checks
tests/test_config_exporters.py
[error] 448-448: Ruff check failed. Unused import: F401 in tests/test_config_exporters.py: 'from mcp_agent.config import get_settings'. Remove unused import. 1 fixable with --fix. Command: ruff check
🔇 Additional comments (7)
examples/tracing/langfuse/mcp_agent.secrets.yaml.example (1)
11-15: No action required – otel.exporters lists are concatenated via deep_merge in get_settings (and covered by test_merge_otel_exporters_from_config_and_secrets).tests/test_config_exporters.py (1)
1-603: Excellent test coverage for multi-version schema support.The test suite comprehensively validates:
- V1 string exporters with legacy fields
- V2 type-discriminated format with
typefield- V3 key-discriminated format with dict keys
- Cross-version compatibility and mixed configurations
- Config/secrets merging behavior for exporters
- Error handling for invalid formats
The assertion helpers (
_assert_console_exporter,_assert_file_exporter,_assert_otlp_exporter) provide clear, reusable validation logic for the V3 dict-based format.src/mcp_agent/config.py (5)
537-562: LGTM: Clean exporter settings models.The new exporter settings classes (
ConsoleExporterSettings,FileExporterSettings,OTLPExporterSettings) are well-designed with appropriate fields and proper configuration. TheOpenTelemetryExporterSettingsunion alias provides clear type semantics.
572-596: Flexible type definition supports multi-version schemas.The
exporterstype definition accommodates all three schema versions (V1 strings, V2 type-discriminated, V3 key-discriminated) while maintaining backward compatibility. The| Dictannotations allow initial dictionary parsing during YAML deserialization before concrete types are determined in the validators.The documentation clearly explains the supported formats with helpful examples for each version.
Based on past review comments, the
| Dictannotation was discussed and justified for YAML deserialization needs.
607-672: LGTM: Comprehensive schema normalization.The
_coerce_exporters_schemavalidator effectively normalizes the three exporter formats:
- V1 string exporters preserved for finalization with legacy fields
- V2 type-discriminated format
{type: "X", ...}converted to V3{X: {...}}- V3 key-discriminated format preserved as-is
- BaseModel instances handled correctly
The error message at lines 664-668 clearly describes expected formats, which will help users understand validation failures.
674-788: LGTM: Proper finalization and legacy field handling.The
_finalize_exportersvalidator correctly:
- Extracts and normalizes legacy top-level fields (
path,path_settings,otlp_settings)- Converts all exporters to uniform V3 key-discriminated dict format
- Applies legacy field fallbacks for V1 string exporters
- Validates typed settings models for each exporter type
- Removes legacy fields to prevent leakage in serialization
The conversion ensures that re-serialization and re-validation work correctly with the V3 format, maintaining consistency across config persistence operations.
1059-1085: LGTM: Correct exporter list concatenation for config/secrets merging.The updated
deep_mergefunction properly implements the special-case handling forotel.exporterslists:
- Uses path tracking to precisely identify
("otel", "exporters")context- Concatenates exporter lists instead of replacing them
- Maintains standard merge behavior for other list types
- Enables combining exporters from config.yaml and secrets.yaml
This implementation matches the merge behavior validated in the test suite and supports the use case described in the PR comments where config and secrets each specify different OTLP exporters that should both be active.
alienzach
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_config_exporters.py (2)
6-11: DRY the deep_merge helper; import the canonical implementation.Avoid duplicating merge logic in tests; import deep_merge from mcp_agent.config and remove both local definitions. Keeps tests aligned with production behavior and reduces drift.
Imports update:
from mcp_agent.config import ( OpenTelemetrySettings, Settings, TraceOTLPSettings, TracePathSettings, + deep_merge, )Remove local helper in test_merge_otel_exporters_from_config_and_secrets:
- # Manually perform deep merge as get_settings does internally - def deep_merge(base: dict, update: dict, path: tuple = ()) -> dict: - """Recursively merge two dictionaries, preserving nested structures. - - Special handling for 'exporters' lists under 'otel' key: - - Concatenates lists instead of replacing them - - Allows combining exporters from config and secrets files - """ - merged = base.copy() - for key, value in update.items(): - current_path = path + (key,) - if ( - key in merged - and isinstance(merged[key], dict) - and isinstance(value, dict) - ): - merged[key] = deep_merge(merged[key], value, current_path) - elif ( - key in merged - and isinstance(merged[key], list) - and isinstance(value, list) - and current_path == ("otel", "exporters") - ): - # Concatenate exporters lists from config and secrets - merged[key] = merged[key] + value - else: - merged[key] = value - return mergedRemove local helper in test_merge_non_otel_lists_are_replaced_not_concatenated:
- # Manually perform deep merge as get_settings does internally - def deep_merge(base: dict, update: dict, path: tuple = ()) -> dict: - """Recursively merge two dictionaries, preserving nested structures. - - Special handling for 'exporters' lists under 'otel' key: - - Concatenates lists instead of replacing them - - Allows combining exporters from config and secrets files - """ - merged = base.copy() - for key, value in update.items(): - current_path = path + (key,) - if ( - key in merged - and isinstance(merged[key], dict) - and isinstance(value, dict) - ): - merged[key] = deep_merge(merged[key], value, current_path) - elif ( - key in merged - and isinstance(merged[key], list) - and isinstance(value, list) - and current_path == ("otel", "exporters") - ): - # Concatenate exporters lists from config and secrets - merged[key] = merged[key] + value - else: - merged[key] = value - return mergedAlso applies to: 471-499, 524-551
4-4: Import ValidationError from pydantic for stability.Use pydantic.ValidationError rather than pydantic_core to match public API and future-proof across versions.
Please confirm your pinned Pydantic version; both v1 and v2 expose pydantic.ValidationError.
-from pydantic_core import ValidationError +from pydantic import ValidationError
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_config_exporters.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_config_exporters.py (1)
src/mcp_agent/config.py (5)
OpenTelemetrySettings(565-788)TracePathSettings(498-521)TraceOTLPSettings(524-535)Settings(867-980)deep_merge(1059-1085)
| and current_path == ("otel", "exporters") | ||
| ): | ||
| # Concatenate exporters lists from config and secrets | ||
| merged[key] = merged[key] + value |
There was a problem hiding this comment.
@saqadri @rholinshead call for last review before merge. especially this chunk of update, which touches the common part for merging. Test cases is in test_config_exporter.py:446-602
There was a problem hiding this comment.
This is to handle list specially in merge, instead of replace, we concat them. I currently only let this apply to otel.exporters. But I think in the future, we may need similar merge when we have secrets in list data structure again.
|
@alienzach I passed this diff through GPT-5 and it raised these two valid points:
|
@alienzach fixed |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml (1)
44-48: Update example to show the V3 dictionary-key schema.Now that the docs steer folks toward the V3 representation, let’s keep the example consistent by changing the console entry to
- console: {}(and the commented OTLP example to# - otlp: { endpoint: ... }). That way we demo the modern shape while still keeping console-only defaults.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml(1 hunks)tests/test_config_exporters.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_config_exporters.py (1)
src/mcp_agent/config.py (5)
OpenTelemetrySettings(565-788)TracePathSettings(498-521)TraceOTLPSettings(524-535)Settings(867-980)deep_merge(1059-1085)
🔇 Additional comments (4)
tests/test_config_exporters.py (4)
1-47: LGTM! Clean imports and helper functions.The imports are correct, and the helper assertion functions provide clear, reusable utilities for verifying exporter configurations across different formats.
50-296: LGTM! Comprehensive test coverage for all schema versions.The tests thoroughly validate V1 (string with legacy fields), V2 (type-discriminated), and V3 (key-discriminated) exporter formats. Good coverage of edge cases including BaseModel instances, empty configs, and multiple exporters of the same type.
303-373: LGTM! Cross-version compatibility and error handling are well tested.The tests validate format mixing, automatic conversion, and proper cleanup of legacy fields. Error tests correctly expect
ValidationErroras noted in previous review feedback.
380-443: LGTM! Integration tests validate end-to-end behavior.The tests successfully verify that all three schema versions work correctly when loaded through the full
Settingsmodel, including proper handling of additional fields likeservice_nameandsample_rate.
| def test_merge_otel_exporters_from_config_and_secrets(): | ||
| """Test that OTEL exporters from config.yaml and secrets.yaml are merged together.""" | ||
|
|
||
| # Simulate config.yaml with one OTLP exporter (public endpoint) | ||
| config_data = { | ||
| "otel": { | ||
| "exporters": [ | ||
| {"otlp": { | ||
| "endpoint": "https://us.cloud.langfuse.com/api/public/otel/v1/traces", | ||
| "headers": {"Authorization": "Basic AUTH_STRING"} | ||
| }} | ||
| ] | ||
| } | ||
| } | ||
|
|
||
| # Simulate secrets.yaml with another OTLP exporter (secret endpoint) | ||
| secrets_data = { | ||
| "otel": { | ||
| "enabled": True, | ||
| "exporters": [ | ||
| {"otlp": {"endpoint": "https://some-other-otel-exporter"}} | ||
| ] | ||
| } | ||
| } | ||
|
|
||
| # Manually perform deep merge as get_settings does internally | ||
| def deep_merge(base: dict, update: dict, path: tuple = ()) -> dict: | ||
| """Recursively merge two dictionaries, preserving nested structures. | ||
| Special handling for 'exporters' lists under 'otel' key: | ||
| - Concatenates lists instead of replacing them | ||
| - Allows combining exporters from config and secrets files | ||
| """ | ||
| merged = base.copy() | ||
| for key, value in update.items(): | ||
| current_path = path + (key,) | ||
| if ( | ||
| key in merged | ||
| and isinstance(merged[key], dict) | ||
| and isinstance(value, dict) | ||
| ): | ||
| merged[key] = deep_merge(merged[key], value, current_path) | ||
| elif ( | ||
| key in merged | ||
| and isinstance(merged[key], list) | ||
| and isinstance(value, list) | ||
| and current_path == ("otel", "exporters") | ||
| ): | ||
| # Concatenate exporters lists from config and secrets | ||
| merged[key] = merged[key] + value | ||
| else: | ||
| merged[key] = value | ||
| return merged | ||
|
|
||
| merged = deep_merge(config_data, secrets_data) | ||
| settings = Settings(**merged) | ||
|
|
||
| # Verify both exporters are present | ||
| assert settings.otel.enabled is True | ||
| assert len(settings.otel.exporters) == 2 | ||
|
|
||
| # Verify first exporter (from config.yaml) | ||
| _assert_otlp_exporter( | ||
| settings.otel.exporters[0], | ||
| endpoint="https://us.cloud.langfuse.com/api/public/otel/v1/traces", | ||
| headers={"Authorization": "Basic AUTH_STRING"} | ||
| ) | ||
|
|
||
| # Verify second exporter (from secrets.yaml) | ||
| _assert_otlp_exporter( | ||
| settings.otel.exporters[1], | ||
| endpoint="https://some-other-otel-exporter" | ||
| ) | ||
|
|
||
|
|
||
| def test_merge_non_otel_lists_are_replaced_not_concatenated(): | ||
| """Test that non-OTEL lists are replaced, not concatenated (default behavior).""" | ||
| # Manually perform deep merge as get_settings does internally | ||
| def deep_merge(base: dict, update: dict, path: tuple = ()) -> dict: | ||
| """Recursively merge two dictionaries, preserving nested structures. | ||
| Special handling for 'exporters' lists under 'otel' key: | ||
| - Concatenates lists instead of replacing them | ||
| - Allows combining exporters from config and secrets files | ||
| """ | ||
| merged = base.copy() | ||
| for key, value in update.items(): | ||
| current_path = path + (key,) | ||
| if ( | ||
| key in merged | ||
| and isinstance(merged[key], dict) | ||
| and isinstance(value, dict) | ||
| ): | ||
| merged[key] = deep_merge(merged[key], value, current_path) | ||
| elif ( | ||
| key in merged | ||
| and isinstance(merged[key], list) | ||
| and isinstance(value, list) | ||
| and current_path == ("otel", "exporters") | ||
| ): | ||
| # Concatenate exporters lists from config and secrets | ||
| merged[key] = merged[key] + value | ||
| else: | ||
| merged[key] = value | ||
| return merged | ||
|
|
||
| # Test with logger.transports (should be replaced, not concatenated) | ||
| config_data = { | ||
| "logger": { | ||
| "transports": ["console", "file"] | ||
| } | ||
| } | ||
| secrets_data = { | ||
| "logger": { | ||
| "transports": ["http"] | ||
| } | ||
| } | ||
| merged = deep_merge(config_data, secrets_data) | ||
| # Should be replaced, not concatenated | ||
| assert merged["logger"]["transports"] == ["http"] | ||
| assert len(merged["logger"]["transports"]) == 1 | ||
|
|
||
| # Test with mcp.servers (dict, should be merged) | ||
| config_data = { | ||
| "mcp": { | ||
| "servers": { | ||
| "fetch": {"command": "uvx", "args": ["mcp-server-fetch"]} | ||
| } | ||
| } | ||
| } | ||
| secrets_data = { | ||
| "mcp": { | ||
| "servers": { | ||
| "filesystem": {"command": "npx", "args": ["-y", "@modelcontextprotocol/server-filesystem"]} | ||
| } | ||
| } | ||
| } | ||
| merged = deep_merge(config_data, secrets_data) | ||
| # Both servers should be present (dicts are merged) | ||
| assert "fetch" in merged["mcp"]["servers"] | ||
| assert "filesystem" in merged["mcp"]["servers"] | ||
|
|
||
| # Test with a nested list that's NOT otel.exporters (should be replaced) | ||
| config_data = { | ||
| "agents": { | ||
| "search_paths": [".claude/agents", "~/.claude/agents"] | ||
| } | ||
| } | ||
| secrets_data = { | ||
| "agents": { | ||
| "search_paths": [".mcp-agent/agents"] | ||
| } | ||
| } | ||
| merged = deep_merge(config_data, secrets_data) | ||
| # Should be replaced, not concatenated | ||
| assert merged["agents"]["search_paths"] == [".mcp-agent/agents"] | ||
| assert len(merged["agents"]["search_paths"]) == 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate duplicate deep_merge implementations.
The deep_merge function is defined identically in both test functions (lines 472-498 and 524-550), duplicating the implementation from src/mcp_agent/config.py (lines 1058-1084). This violates DRY and creates maintenance burden.
Import deep_merge from config.py instead:
+from mcp_agent.config import (
+ OpenTelemetrySettings,
+ Settings,
+ TraceOTLPSettings,
+ TracePathSettings,
+ deep_merge,
+)Then remove the duplicate definitions from both test functions:
def test_merge_otel_exporters_from_config_and_secrets():
"""Test that OTEL exporters from config.yaml and secrets.yaml are merged together."""
# Simulate config.yaml with one OTLP exporter (public endpoint)
config_data = {
"otel": {
"exporters": [
{"otlp": {
"endpoint": "https://us.cloud.langfuse.com/api/public/otel/v1/traces",
"headers": {"Authorization": "Basic AUTH_STRING"}
}}
]
}
}
# Simulate secrets.yaml with another OTLP exporter (secret endpoint)
secrets_data = {
"otel": {
"enabled": True,
"exporters": [
{"otlp": {"endpoint": "https://some-other-otel-exporter"}}
]
}
}
- # Manually perform deep merge as get_settings does internally
- def deep_merge(base: dict, update: dict, path: tuple = ()) -> dict:
- """Recursively merge two dictionaries, preserving nested structures.
-
- Special handling for 'exporters' lists under 'otel' key:
- - Concatenates lists instead of replacing them
- - Allows combining exporters from config and secrets files
- """
- merged = base.copy()
- for key, value in update.items():
- current_path = path + (key,)
- if (
- key in merged
- and isinstance(merged[key], dict)
- and isinstance(value, dict)
- ):
- merged[key] = deep_merge(merged[key], value, current_path)
- elif (
- key in merged
- and isinstance(merged[key], list)
- and isinstance(value, list)
- and current_path == ("otel", "exporters")
- ):
- # Concatenate exporters lists from config and secrets
- merged[key] = merged[key] + value
- else:
- merged[key] = value
- return merged
-
merged = deep_merge(config_data, secrets_data)
settings = Settings(**merged)Apply the same change to test_merge_non_otel_lists_are_replaced_not_concatenated (remove lines 524-550).
🤖 Prompt for AI Agents
In tests/test_config_exporters.py around lines 446 to 602, remove the duplicated
deep_merge function definitions inside
test_merge_otel_exporters_from_config_and_secrets and
test_merge_non_otel_lists_are_replaced_not_concatenated and instead import the
canonical implementation from the package config module; add a single import
like "from mcp_agent.config import deep_merge" at the top of the test file (or
the appropriate import path for your package), delete the two local deep_merge
definitions, and run tests to ensure names and behavior match the original
implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_config_exporters.py (1)
472-498: Extract duplicateddeep_mergefunction to reduce maintenance burden.The
deep_mergefunction is defined identically in two separate tests (lines 472-498 and 524-550). This duplication violates the DRY principle and increases maintenance risk.Option 1 (recommended): Extract to a module-level helper function:
+def _deep_merge_for_testing(base: dict, update: dict, path: tuple = ()) -> dict: + """Recursively merge two dictionaries, preserving nested structures. + + Special handling for 'exporters' lists under 'otel' key: + - Concatenates lists instead of replacing them + - Allows combining exporters from config and secrets files + """ + merged = base.copy() + for key, value in update.items(): + current_path = path + (key,) + if ( + key in merged + and isinstance(merged[key], dict) + and isinstance(value, dict) + ): + merged[key] = _deep_merge_for_testing(merged[key], value, current_path) + elif ( + key in merged + and isinstance(merged[key], list) + and isinstance(value, list) + and current_path == ("otel", "exporters") + ): + # Concatenate exporters lists from config and secrets + merged[key] = merged[key] + value + else: + merged[key] = value + return merged + + def test_merge_otel_exporters_from_config_and_secrets(): """Test that OTEL exporters from config.yaml and secrets.yaml are merged together.""" # ... - def deep_merge(base: dict, update: dict, path: tuple = ()) -> dict: - # ... (remove inline definition) - - merged = deep_merge(config_data, secrets_data) + merged = _deep_merge_for_testing(config_data, secrets_data)Option 2: Import and test the actual implementation from
mcp_agent.config:+from mcp_agent.config import deep_merge + def test_merge_otel_exporters_from_config_and_secrets(): """Test that OTEL exporters from config.yaml and secrets.yaml are merged together.""" # ... - def deep_merge(base: dict, update: dict, path: tuple = ()) -> dict: - # ... (remove inline definition) - merged = deep_merge(config_data, secrets_data)Note: If you choose Option 2, ensure that
deep_mergeis exported frommcp_agent.configor consider importing from the internal location where it's defined.Also applies to: 524-550
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml(1 hunks)schema/mcp-agent.config.schema.json(5 hunks)tests/test_config_exporters.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_config_exporters.py (1)
src/mcp_agent/config.py (5)
OpenTelemetrySettings(565-788)TracePathSettings(498-521)TraceOTLPSettings(524-535)Settings(867-980)deep_merge(1059-1085)
🔇 Additional comments (2)
tests/test_config_exporters.py (1)
1-602: Excellent test coverage and organization!The test suite is comprehensive and well-structured:
- Clear separation of V1, V2, and V3 format tests
- Cross-version compatibility validation
- Error handling for invalid configurations
- Integration tests with the full Settings model
- Merge behavior tests for config/secrets scenarios
The docstrings clearly explain what each test validates, making the test suite easy to maintain and understand.
schema/mcp-agent.config.schema.json (1)
1022-1080: Schema correctly implements multi-version exporter configuration.The
exportersschema properly supports all three configuration formats:
- V1 (string literals):
"console","file","otlp"- V2 (type-discriminated): Handled by validator, not schema-enforced
- V3 (key-discriminated): Correctly uses
propertyNameswithconstto enforce single-key objectsThe use of
propertyNames: {const: "console"}is the correct JSON Schema approach to restrict an object to a single, specific key. This ensures that key-discriminated exporters like{console: {}}or{file: {path: "..."}}are properly validated.The schema maintains backward compatibility by accepting both the key-discriminated format and direct references to typed settings classes.
|
Going to merge this in so we can get this published. Nice work @alienzach! |
2 participants
Refactored OpenTelemetry Exporter Configuration
TL;DR
Refactored OpenTelemetry exporter configuration to support three schema versions with improved backward compatibility.
What changed?
typefield in favor of a more flexible approachexporters: ["console", "file"])exporters: [{type: "file", path: "/tmp/out"}])exporters: [console: {}, file: {path: "trace.jsonl"}])path,path_settings,otlp_settings) for backward compatibilityHow to test?
pytest tests/test_config_exporters.pyWhy make this change?
The previous configuration approach using discriminated unions with a
typefield was overly restrictive and not as user-friendly. This refactoring:Summary by CodeRabbit
New Features
Documentation
Refactor
Tests