Skip to content

feat(observability): add baggage attribute mapping for span customization#4705

Open
vishu-bh wants to merge 7 commits into
mainfrom
feat/baggage-attribute-mapping-4700
Open

feat(observability): add baggage attribute mapping for span customization#4705
vishu-bh wants to merge 7 commits into
mainfrom
feat/baggage-attribute-mapping-4700

Conversation

@vishu-bh
Copy link
Copy Markdown
Collaborator

@vishu-bh vishu-bh commented May 11, 2026

Problem

Issue #4700 requires selected tenant/user context carried through OpenTelemetry baggage to appear in exported spans without the baggage. prefix. Clients can send values such as tenant.id and user.id as OTEL baggage, but some observability backends expect those dimensions as span attributes named tenant.id / user.id, not baggage.tenant.id / baggage.user.id.

The gateway still needs to stay aligned with OTEL semantics: baggage remains the propagation mechanism, while span attributes are the reporting surface.

Solution

This PR replaces the earlier span-processor remapping approach with a baggage emission policy that is applied where baggage is copied onto spans.

The SpanAttributeCustomizer plugin now supports:

config:
  allowed_baggage_span_attributes:
    - "tenant.id"
    - "user.id"
  emit_baggage_prefixed_attributes: false

With that configuration:

  • OTEL baggage continues to carry tenant.id and user.id
  • only allowlisted baggage keys are promoted to span attributes
  • exported span attributes are emitted as tenant.id / user.id
  • baggage.tenant.id / baggage.user.id are not emitted for those allowlisted keys

If the new config is not provided, the gateway preserves legacy behavior and emits baggage as baggage.<key>.

Changes

  • mcpgateway/observability.py

    • Added BaggageSpanAttributePolicy
    • Added startup extraction for the SpanAttributeCustomizer baggage policy
    • Applies the policy directly in both request-root span baggage injection and create_span()
    • Removed the old BaggageAttributeSpanProcessor remapping path
  • mcpgateway/main.py

    • Loads the baggage span attribute policy after plugin config is available and before telemetry is initialized
  • plugins/span_attribute_customizer/config_schema.py

    • Added allowed_baggage_span_attributes
    • Added emit_baggage_prefixed_attributes
    • Added validation/normalization for the baggage allowlist
  • plugins/config.yaml

    • Added ready-to-use baggage policy config for tenant.id and user.id
  • Docs

    • Updated OpenTelemetry architecture docs
    • Updated span attribute reference docs
    • Updated SpanAttributeCustomizer docs and README
  • Tests

    • Added coverage for policy extraction, malformed config handling, allowlist behavior, default legacy behavior, and both baggage-to-span injection paths

Testing

Validated locally:

make pre-commit

Also verified focused observability coverage and diff-cover behavior locally after adding tests for the changed baggage paths.

Fixes #4700

@vishu-bh vishu-bh self-assigned this May 12, 2026
@vishu-bh vishu-bh force-pushed the feat/baggage-attribute-mapping-4700 branch from f0531a0 to d64eabc Compare May 12, 2026 09:23
@ja8zyjits
Copy link
Copy Markdown
Member

ja8zyjits commented May 12, 2026

Minor Issues

1. Missing Inline Comment ⚠️

Location: mcpgateway/main.py:1478-1480

Issue: Telemetry init reordering not explained.

Recommendation: Add comment:

# Extract span attribute mapping from plugin config before telemetry init
span_attribute_mapping = extract_span_attribute_mapping(get_plugin_manager_factory())
# Initialize telemetry with mapping (must run after plugin manager init)
init_telemetry(span_attribute_mapping=span_attribute_mapping)

Priority: Low (code is correct, just needs documentation)

2. Missing User Documentation ⚠️

Issue: No user-facing documentation for attribute_mapping configuration.

Missing:

  1. Plugin configuration guide
  2. Example in plugin README
  3. Mention in observability documentation

Recommendation: Create follow-up issue for documentation.

Priority: Medium (feature works but users need guidance)

Optional Enhancements

3. Integration Test (Optional)

Suggestion: Add integration test with actual baggage propagation end-to-end.

Current Coverage: Unit tests cover processor logic thoroughly.

Value: Would verify full pipeline (baggage injection → processor → export).

Priority: Low (nice-to-have, not blocking)

Final Verdict

✅ APPROVE with Minor Recommendations

Strengths:

  1. ✅ Correct architectural approach (SpanProcessor is right layer)
  2. ✅ Defensive implementation (fail-closed, validates input)
  3. ✅ Comprehensive test coverage (13 test cases, all edge cases)
  4. ✅ Backward compatible (additive feature)
  5. ✅ Addresses root cause (timing issue with baggage injection)
  6. ✅ Safe and performant (minimal overhead)

Minor Issues:

  1. ⚠️ Add inline comment explaining telemetry init ordering
  2. ⚠️ Missing user-facing documentation (create follow-up issue)

Copy link
Copy Markdown
Member

@ja8zyjits ja8zyjits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@ja8zyjits ja8zyjits added the do-not-merge Do NOT merge. This requires manual effort, with multiple checks! label May 13, 2026
Copy link
Copy Markdown
Member

@ja8zyjits ja8zyjits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testing needed.

@vishu-bh vishu-bh force-pushed the feat/baggage-attribute-mapping-4700 branch 2 times, most recently from fc315b9 to 211f91b Compare May 14, 2026 13:10
vishu-bh added 7 commits May 14, 2026 15:26
…tion

Implements Option A semantics for SpanAttributeCustomizer plugin:
- Extract attribute_mapping from plugin config at startup
- Add BaggageAttributeSpanProcessor to duplicate baggage.* attributes
- Initialize telemetry after plugin manager to access config
- Add comprehensive test coverage for mapping extraction and processor

This allows operators to remap baggage-derived span attributes like
baggage.tenant.id to custom keys like tenant.id for backend compatibility.

Fixes #4700

Signed-off-by: Bob Shell <[email protected]>
Signed-off-by: Vishu Bhatnagar <[email protected]>
- Add test for extract_span_attribute_mapping with None factory
- Add test for BaggageAttributeSpanProcessor with empty mapping
- Add test for BaggageAttributeSpanProcessor with None span
- Add test for BaggageAttributeSpanProcessor skipping None values
- Add test for BaggageAttributeSpanProcessor on_end returning None

These tests cover lines 416, 468, 478, and 487 in observability.py,
bringing coverage from 92.3% to above the 93% threshold.

Relates-to: #4700
Signed-off-by: Vishu Bhatnagar <[email protected]>
- Prefix unused 'span' parameter with underscore in observability.py
- Remove redundant SessionLocal import in admin.py

Fixes pylint unused-argument and reimported warnings.

Signed-off-by: Vishu Bhatnagar <[email protected]>
- Set OTEL_BAGGAGE_HEADER_MAPPINGS=x-tenant-id:tenant.id in Langfuse compose

- Enables testing of tenant.id propagation to span attributes via baggage

Signed-off-by: Vishu Bhatnagar <[email protected]>
Signed-off-by: Vishu Bhatnagar <[email protected]>
Explain why span_attribute_mapping extraction happens before
telemetry initialization - it must run after plugin manager init
to access plugin configuration.

Addresses review comment about missing documentation for
telemetry init reordering logic.

Signed-off-by: Vishu Bhatnagar <[email protected]>
Signed-off-by: Vishu Bhatnagar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge Do NOT merge. This requires manual effort, with multiple checks!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: SpanAttributeCustomizer plugin cannot rename baggage-derived span attributes

2 participants