-
Notifications
You must be signed in to change notification settings - Fork 310
JBRes-7677: opentelementry support for MCP #1421
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
base: develop
Are you sure you want to change the base?
Conversation
b8a8889 to
4eea75b
Compare
1d40250 to
1633b4a
Compare
cdddf62 to
32ca582
Compare
tiginamaria
left a 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.
Thank you! Looks good to me, but I need @sdubov's second independent opinion here
| public val argsSerializer: KSerializer<TArgs>, | ||
| public val resultSerializer: KSerializer<TResult>, | ||
| public val descriptor: ToolDescriptor, | ||
| private val meta: Map<KClass<*>, Any> = emptyMap(), |
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.
For me it's fine like that (better then string or storage key), but need @sdubov oppinion here
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.
Also not sure how it will work with java api
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 have couple notes here:
- I would name it
metaInfoto match the naming withMessageclass that we use. - I'm not sure how this approach should be extended if we would like to pass this meta info through network. I would assume that this should be serrializable class. Otherwise, this data is only for for internal usage.
- How would that work with Java API?
- What is the problem with key-value approach here?
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.
For key-value you mean string-string? It bas another option, but It's not type-safe and if I want MCP+ACP tool (for example) and it could cause key clashing. On the other hand it's easy to serialize and it's for sure java-friendly
|
|
||
| /** | ||
| * A constant representing an empty registry with no tools. | ||
| * TODO: ToolRegistry is mutable but stored as an immutable object. |
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.
Maybe put deprecation with empty() replacement?
| * @property sessionId Unique identifier for the current MCP session, if session management is enabled. | ||
| */ | ||
| @InternalAgentsApi | ||
| public class McpServerMeta( |
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.
For @sdubov: this was added to the core intentionally, as we don't wnat Otel dependend on MCP
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.
Folks, as I see the metadata is used not for features, but for tool registry. I have a proposal to discuss here: What do you think if we add a general metadata key-value property (not directly referring MCP parameters) for our internal tools in the core module. MCP adds special MCP-related attributes to this metadata. We add this metadata to tool events, OTel will read this metadata and add necessary attributes to spans converting all or filtered MCP data into spans. WDYT?
sdubov
left a 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.
@vova-jb, thank you for your implementation. I looked through general approach and through details. Please see my comments in review. I would like to discuss two general questions that was raised in my mind:
- How do we compose / wrap spans for MCP vs regular tool call spans?
- How do we present meta information for tools?
I would also add someone from the team to discuss this together if you do not mind.
| agentConfig: AIAgentConfig, | ||
| strategy: AIAgentGraphStrategy<Input, Output>, | ||
| toolRegistry: ToolRegistry = ToolRegistry.EMPTY, | ||
| toolRegistry: ToolRegistry = ToolRegistry.empty(), |
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.
@vova-jb, good point. I think it worth creating a separate ticket and separate update though
| * @property sessionId Unique identifier for the current MCP session, if session management is enabled. | ||
| */ | ||
| @InternalAgentsApi | ||
| public class McpServerMeta( |
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.
Folks, as I see the metadata is used not for features, but for tool registry. I have a proposal to discuss here: What do you think if we add a general metadata key-value property (not directly referring MCP parameters) for our internal tools in the core module. MCP adds special MCP-related attributes to this metadata. We add this metadata to tool events, OTel will read this metadata and add necessary attributes to spans converting all or filtered MCP data into spans. WDYT?
| public val instructions: String?, | ||
| public val mcpProtocolVersion: String, | ||
| public val mcpTransportType: McpTransportType, | ||
| public val sessionId: String?, |
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.
We have session Id inside the framework. Maybe name it mcpSessionId to not mess with Koog session id?
| public class McpServerMeta( | ||
| public val serverUrl: String?, | ||
| public val serverPort: Int?, | ||
| public val siteUrl: String?, |
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.
Sorry, could you please clarify the difference between serverUrl and siteUrl? I've read the properties KDoc, but I feel that I not exactly got it.
| * This transport is typically used for local MCP servers running as separate | ||
| * processes that communicate via stdio. Suitable for development and local tools. | ||
| */ | ||
| Pipe("pipe"), |
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.
@vova-jb, is not it called "stdio" mcp transport type by MCP providers?
| import kotlin.test.assertTrue | ||
| import kotlin.time.Duration.Companion.seconds | ||
|
|
||
| @InternalAgentsApi |
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.
Could you please clarify why did you mark a test class as InternalAgentToolsApi. This annotation is used for internal methods that we explicitly mark for a users to show that it is not a public API.
| import kotlin.time.Duration.Companion.seconds | ||
|
|
||
| @Execution(ExecutionMode.SAME_THREAD) | ||
| @InternalAgentsApi |
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.
Same comment as above for tests.
| public val argsSerializer: KSerializer<TArgs>, | ||
| public val resultSerializer: KSerializer<TResult>, | ||
| public val descriptor: ToolDescriptor, | ||
| private val meta: Map<KClass<*>, Any> = emptyMap(), |
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 have couple notes here:
- I would name it
metaInfoto match the naming withMessageclass that we use. - I'm not sure how this approach should be extended if we would like to pass this meta info through network. I would assume that this should be serrializable class. Otherwise, this data is only for for internal usage.
- How would that work with Java API?
- What is the problem with key-value approach here?
| * 1. Set up a Langfuse project and credentials as described [here](https://langfuse.com/docs/get-started#create-new-project-in-langfuse) | ||
| * 2. Get Langfuse credentials as described [here](https://langfuse.com/faq/all/where-are-langfuse-api-keys) | ||
| * 3. Set `LANGFUSE_HOST`, `LANGFUSE_PUBLIC_KEY`, and `LANGFUSE_SECRET_KEY` environment variables | ||
| * 4. Set `OPENAI_API_KEY` from [here](https://platform.openai.com/account/api-keys) |
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.
Changes in this file seem unrelated to the code from the PR. Maybe we can do it separately if needed?
| # | ||
| LANGFUSE_SECRET_KEY=sk-lf-... | ||
| LANGFUSE_PUBLIC_KEY=pk-lf-... | ||
| LANGFUSE_HOST=http://localhost:3000 |
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.
Could you please skip the value part as it is defined for other env vars to keep it consistent?
32ca582 to
f16df01
Compare
Motivation and Context
Mcp support for opentelemetry metrics
Breaking Changes
none
Type of the changes