Skip to content

Backfills trace injection test and migrates from request.params to request.params._meta #360

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

Merged
merged 2 commits into from
Apr 29, 2025

Conversation

codefromthecrypt
Copy link
Contributor

Motivation and Context

Before, trace IDs were encoded in the request.params object which co-mingled it with fields like tool arguments.

This backfills an encoding test and moves it to request.params._meta which is used elsewhere and proposed as a convention due to prior art and also being a bit safer for ad-hoc implementation regardless of this implementation being careful.

This also corrects logic to replace any existing key in the message however unlikely that might be.

How Has This Been Tested?

Backfilled a unit test

Breaking Changes

The choice to use request.params was released in https://github.com/modelcontextprotocol/csharp-sdk/releases/tag/v0.1.0-preview.9. This changes it to request.params._meta. This means that applications already releasing this will need to update both sides to have context propagation. Since this SDK is in pre-release, I didn't add logic to fall back to extract from request.params

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@stephentoub
Copy link
Contributor

cc: @lmolkova, @samsp-msft

Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@samsp-msft samsp-msft left a comment

Choose a reason for hiding this comment

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

woot!

@stephentoub stephentoub merged commit 17a9427 into modelcontextprotocol:main Apr 29, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants