Skip to content

Draft : Add agent astronomy shop and astronomy shop MCP server#3148

Draft
fali007 wants to merge 15 commits intoopen-telemetry:mainfrom
fali007:main
Draft

Draft : Add agent astronomy shop and astronomy shop MCP server#3148
fali007 wants to merge 15 commits intoopen-telemetry:mainfrom
fali007:main

Conversation

@fali007
Copy link
Copy Markdown

@fali007 fali007 commented Mar 24, 2026

Changes

This is a Draft PR to demo changes to address #3140

Please provide a brief description of the changes here.

Merge Requirements

For new features contributions, please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs
  • Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

Signed-off-by: felixgeorge <felix.george@ibm.com>
@github-actions github-actions Bot added the helm-update-required Requires an update to the Helm chart when released label Mar 24, 2026
fali007 added 5 commits March 24, 2026 16:46
Signed-off-by: felixgeorge <felix.george@ibm.com>
Signed-off-by: felixgeorge <felix.george@ibm.com>
Signed-off-by: felixgeorge <felix.george@ibm.com>
Signed-off-by: felixgeorge <felix.george@ibm.com>
Signed-off-by: felixgeorge <felix.george@ibm.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 2, 2026

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Copy link
Copy Markdown
Contributor

@ps48 ps48 left a comment

Choose a reason for hiding this comment

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

@fali007 Added some initial set of comments. Also can you please rebase you PR on top of main to remove the conflicts!

Comment thread src/mcp/src/mcp_server/tools.py Outdated
@@ -0,0 +1,140 @@
#!/usr/bin/python
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like this is a dupe for src/agents/tools.py Can we just keep one?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure. Made the change

Comment thread src/agent/src/agents/tools.py Outdated

def convert_currency(from_currency: str, to_currency: str, amount: float):
"""Convert between currencies."""
url = f"http://{BASE_URL}/api/currency/convert?from={from_currency}&to={to_currency}&amount={amount}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where is this route hosted? I see that the src/frontend/pages/api/currency.ts is the only handler to GET returning currencyCodes[]`. Currency conversion is done internally via gRPC (CurrencyGateway.convert) and is never exposed over HTTP.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed the tool

Comment thread src/agent/src/agents/tools.py Outdated

def get_shipping_quote(address_id: str):
"""Get estimated shipping cost for a given address."""
url = f"http://{BASE_URL}/api/shipping/quote/{address_id}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/api/shipping/quote/{address_id} does not exist. The actual endpoint is GET /api/shipping with query parameters itemList (JSON-stringified CartItem[]), currencyCode, and address (JSON-stringified Address). Should be something like:

url = f"http://{BASE_URL}/api/shipping"
params = {"itemList": json.dumps(items), "currencyCode": currency, "address": json.dumps(address)}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Made the change

Comment thread src/mcp/src/mcp_server/tools.py Outdated

def get_cart():
"""Retrieve the current contents of a user's cart."""
url = f"http://{BASE_URL}/api/cart"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need session id here

const { userId, items } = await CartGateway.getCart(sessionId as string);

  def get_cart(sessionId: str):
      url = f"http://{BASE_URL}/api/cart"
      res = requests.get(url, params={"sessionId": sessionId})

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added session_id as request param

Comment thread src/agent/src/agents/llm.py Outdated
if use_vcr:
cassette_name = f"{model_name.replace('/', '_')}_casette.yaml"
if "http_async_client" not in kwargs:
kwargs["http_async_client"] = httpx.AsyncClient(verify=False)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we gate this behind an env var (e.g., LLM_TLS_VERIFY). ? LLM provider is external is usually external so should we default to verify=True

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added behind env variable.

Comment thread docker-compose.yml Outdated
Comment on lines +867 to +872
jaeger:
condition: service_started
otel-collector:
condition: service_started
product-catalog:
condition: service_started
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The chatbot only talks to the agent service, but its depends_on lists jaeger, otel-collector, and product-catalog. It should depend on agent instead.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Corrected the dependency for MCP and chatbot

Comment thread .env Outdated
AGENT_PORT=8010
CHATBOT_PORT=7860
LLM_BASE_URL=
LLM_MODEL=Azure/gpt-4o
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LLM_MODEL=Azure/gpt-4o the OpenTelemetry demo shouldn't default to a vendor-specific model. Please consider leaving this empty or using a placeholder with a comment explaining how to configure it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed

Comment thread src/agent/src/agents/tools.py Outdated
return f"Error fetching ads: {e}"


def add_to_cart(userId: str, productId: str, quantity: int = 1):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The parameter names mix camelCase and snake_case: userId, productId (camelCase) vs product_id, from_currency, to_currency (snake_case). Since these become tool parameter names visible to the LLM, inconsistency can cause the agent to use wrong names. Please pick one convention (snake_case is Pythonic).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed

Comment thread src/chatbot/run.py Outdated


async def start_servers():
"""Run both the LangGraph Agent and the MCP server concurrently."""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like a stale docstring. We can say this runs the chat UI server?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Made the change

Comment thread src/mcp/run.py Outdated


async def start_servers():
"""Run both the LangGraph Agent and the MCP server concurrently."""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here, This looks like a stale docstring. We can say this runs the MCP server?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Made the change

fali007 and others added 5 commits April 20, 2026 19:33
Signed-off-by: felixgeorge <felix.george@ibm.com>
Signed-off-by: felixgeorge <felix.george@ibm.com>
Signed-off-by: felixgeorge <felix.george@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

helm-update-required Requires an update to the Helm chart when released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants