fix(agentgateway)!: remove telemetry source attribution from _lob.py#228
Closed
tiagoek wants to merge 37 commits into
Closed
fix(agentgateway)!: remove telemetry source attribution from _lob.py#228tiagoek wants to merge 37 commits into
tiagoek wants to merge 37 commits into
Conversation
Add environment-based authentication (no mTLS) to support transparent TLS mode where the gateway handles mTLS externally and the SDK uses standard HTTPS with client credentials. Changes: - Add TlsMode enum (STANDARD, TRANSPARENT) to config.py - Make certificate/private_key optional in CustomerCredentials model - Add detect_transparent_credentials() to check for transparent mode env vars - Add load_customer_credentials_from_env() for environment-based credentials - Add _request_token_transparent() for standard HTTPS token requests - Add automatic TLS mode detection in AgentGatewayClient - Add comprehensive tests for transparent mode authentication flow Environment variables for transparent mode: - INTEGRATION_CLIENT_ID - INTEGRATION_TOKEN_URL or INTEGRATION_TOKEN_SERVICE_URL - INTEGRATION_GATEWAY_URL - INTEGRATION_DEPENDENCIES (JSON array) The SDK automatically detects the mode based on environment variable presence and falls back to STANDARD mode (file-based mTLS) when not present.
Add support for targeted MCP server discovery, flexible tool invocation, and enhanced credential detection to support multiple deployment scenarios. New features: - ORD ID filtering: Target specific MCP servers via ord_id parameter in list_mcp_tools() and call_mcp_tool() - Tool-by-name invocation: Accept tool name as string in call_mcp_tool() for more flexible tool invocation patterns - servicebinding.io support: 3-tier credential detection priority (explicit override → servicebinding.io scan → legacy default) - Centralized MCP session management via _mcp_session.py - Abstract IntegrationDependenciesResolver for flexible dependency sources - AgentGatewayServerError exception for better error handling Key implementations: - _resolve_dependency() for ORD ID lookup in integration dependencies - EnvironmentDependenciesResolver for INTEGRATION_DEPENDENCIES env var - _resolve_tool_by_name() helper for string-based tool resolution - SERVICE_BINDING_ROOT scanning for integration-credentials type Backward compatibility: - All existing APIs preserved (get_ias_client_id, etc.) - ord_id parameter optional (defaults to None) - Tool parameter accepts both MCPTool object and string This enables deployment flexibility across Kyma, Kubernetes with service bindings, and environment-variable-based configurations.
Fix MCP client API usage to match streamablehttp_client signature changes
in newer MCP library versions. The library no longer accepts http_client
as a parameter; headers must be passed directly.
Changes:
- Update MCP client API calls in _customer.py and _lob.py
- Remove nested httpx.AsyncClient context managers
- Pass headers directly to streamablehttp_client()
- Upgrade mcp dependency to >=1.20.0 for protocol 2025-11-25 support
- Add proper exception chaining (raise ... from e) per coding guidelines
- Add AGW_DISABLE_SSL_VERIFY environment variable for testing
- Fix indentation errors in _lob.py from previous edits
- Update test_lob.py for API changes
Breaking change in MCP library:
Old: streamablehttp_client(url, http_client=client)
New: streamablehttp_client(url, headers={...}, timeout=...)
Testing support:
Set AGW_DISABLE_SSL_VERIFY=1 to bypass SSL verification when testing
with self-signed certificates. Works in both STANDARD and TRANSPARENT modes.
Migrate from deprecated streamablehttp_client() to the new recommended
streamable_http_client() API (note the underscore). This aligns with
MCP library best practices and provides better separation of concerns
for HTTP client configuration.
Changes:
- Update imports to use streamable_http_client (with underscore)
- Wrap HTTP configuration in httpx.AsyncClient instances
- Pass pre-configured client to streamable_http_client via http_client parameter
- Fix indentation in nested async context managers
- Update test mocks to patch correct function name
- Update test data format to match flat INTEGRATION_DEPENDENCIES structure
API pattern change:
OLD (deprecated):
async with streamablehttp_client(url, headers={...}, timeout=...) as (read, write, _):
NEW (recommended):
async with httpx.AsyncClient(headers={...}, timeout=...) as client:
async with streamable_http_client(url, http_client=client) as (read, write, _):
This provides cleaner architecture where HTTP concerns (headers, timeouts, auth)
are handled by httpx.AsyncClient, and MCP transport focuses on protocol handling.
All 202 tests passing.
Move OpenTelemetry packages from required dependencies to optional [telemetry] extras group. This resolves version conflicts when the auto-instrumentation operator injects its own OTel packages. Problem: - Auto-instrumentation operator provides OTel SDK 1.20.0 - SDK previously required OTel SDK 1.42.1 - Version mismatch caused import errors: ModuleNotFoundError: opentelemetry.exporter.otlp.proto.common._exporter_metrics Solution: - Move all opentelemetry-* dependencies to optional [telemetry] group - Use relaxed version constraints (>=1.20.0) for compatibility - SDK now works with operator-provided OTel packages - Standalone users can install with: pip install sap-cloud-sdk[telemetry] Changed dependencies: - opentelemetry-api (>=1.20.0) - moved to [telemetry] - opentelemetry-sdk (>=1.20.0) - moved to [telemetry] - opentelemetry-exporter-otlp-proto-grpc (>=1.20.0) - moved to [telemetry] - opentelemetry-exporter-otlp-proto-http (>=1.20.0) - moved to [telemetry] - opentelemetry-processor-baggage (>=0.50.0) - moved to [telemetry] - traceloop-sdk (>=0.50.0) - moved to [telemetry] - opentelemetry-instrumentation-langchain (>=0.50.0) - moved to [telemetry] Backward compatibility: - No breaking changes for users who install the SDK standalone - Auto-instrumentation scenarios now work without conflicts - All 202 tests passing Fixes OpenTelemetry version conflict in containerized environments with auto-instrumentation operators.
Fix unconditional telemetry imports that caused ModuleNotFoundError when OpenTelemetry packages were not installed (even though marked as optional). Problem: - Making OTel optional in pyproject.toml wasn't enough - Code still had unconditional imports: `from sap_cloud_sdk.core.telemetry import ...` - When agentgateway imports destination module, it fails if OTel not installed - Error: ModuleNotFoundError: opentelemetry.exporter.otlp.proto.common._exporter_metrics Root Cause Analysis: - destination/client.py imports telemetry unconditionally (line 9) - agentgateway/_lob.py imports from destination (line 16) - Import chain fails when OTel packages missing Solution: 1. Created _telemetry_compat.py module with conditional import pattern 2. Updated destination module files to use conditional imports: - destination/client.py - destination/fragment_client.py - destination/certificate_client.py 3. When telemetry unavailable, provides no-op implementations Pattern: # Old (unconditional): from sap_cloud_sdk.core.telemetry import Module, Operation, record_metrics # New (conditional): from sap_cloud_sdk.core._telemetry_compat import Module, Operation, record_metrics # _telemetry_compat.py handles the try/except and provides fallbacks Benefits: - SDK works with or without telemetry packages installed - Auto-instrumentation operator scenarios now work - No-op decorators when telemetry unavailable (zero overhead) - Standalone installations with [telemetry] extra get full functionality Testing: - All 202 tests passing - Agent import chain works without OTel packages - Backward compatible with telemetry-enabled installations Fixes import error chain: tools.debugging → sap_cloud_sdk.agentgateway → destination → telemetry ❌ tools.debugging → sap_cloud_sdk.agentgateway → destination → _telemetry_compat ✅ Related: fix(telemetry): make OpenTelemetry dependencies optional (c7dc4f9)
Changes: - Add transparent TLS mode with INTEGRATION_AUTH_URL environment variable - Restore servicebinding.io credential discovery from KoblerS branch - Revert MCP API to httpx.AsyncClient wrapper pattern for minimal changes - Add defensive null checks for MCP tool invocation results - Remove AGW_DISABLE_SSL_VERIFY support - Bump version to 0.33.5 Transparent TLS Mode: - Supports INTEGRATION_CLIENT_ID, INTEGRATION_AUTH_URL, INTEGRATION_GATEWAY_URL - No mTLS certificates needed - gateway handles TLS externally - Uses standard HTTPS for token requests ServiceBinding.io Support: - Scans $SERVICE_BINDING_ROOT (default /bindings) for integration-credentials - Falls back to legacy /etc/ums/credentials/credentials path - Supports AGW_CREDENTIALS_PATH override MCP Changes: - Restored httpx.AsyncClient wrapper in _customer.py and _lob.py - Fixed import: streamable_http_client (not streamablehttp_client) - Added null result checks to prevent AttributeError on MCP failures Tests: All 202 agentgateway unit tests pass
- Move OTel packages back to required dependencies (not optional) - Use SAP upstream pinned versions (~=1.42.1, ~=0.61.0) - Revert mcp version constraint from >=1.20.0 to >=1.1.0 - Remove optional telemetry dependency group This matches SAP/cloud-sdk-python main branch exactly for OTel deps.
Move OTel packages to optional 'telemetry' dependency group to support Kubernetes OpenTelemetry auto-instrumentation operators that inject their own OTel version (e.g., 1.20.0) at runtime. Changes: - Remove OTel packages from required dependencies - Add 'telemetry' optional dependency group with relaxed version constraints - Use >=1.20.0 instead of ~=1.42.1 to avoid conflicts with operator-injected versions Installation: - Base: pip install sap-cloud-sdk (no OTel) - With telemetry: pip install sap-cloud-sdk[telemetry] This resolves version conflicts when the K8s operator injects OTel 1.20.0 while the SDK required 1.42.1.
Restore OpenTelemetry version constraints that matched the working Archive 5: - opentelemetry-processor-baggage~=0.61b0 (was >=0.50.0) - opentelemetry-exporter-otlp-proto-grpc~=1.42.1 (was >=1.20.0) - opentelemetry-exporter-otlp-proto-http~=1.42.1 (was >=1.20.0) - traceloop-sdk~=0.61.0 (was >=0.50.0) - opentelemetry-instrumentation-langchain>=0.61.0 (was >=0.50.0) Keep api/sdk with >=1.42.1 as in SAP upstream.
Restore exact Archive 5 / SAP v0.25.1 configuration: - OpenTelemetry packages in REQUIRED dependencies (not optional) - Version: 0.33.5 → 0.33.7 - OTel versions: ~=1.42.1, ~=0.61b0, >=1.42.1 (working config) This matches the working Archive 5 configuration with: - Transparent TLS mode support - servicebinding.io credential discovery - MCP API with httpx.AsyncClient wrapper - All OTel packages always installed All 202 tests pass.
- Fixed IndentationError at line 685 in _customer.py - ClientSession async with block was not properly indented inside streamable_http_client context - Rebuilt wheel with version 0.33.7 containing the fix
- Updated pyproject.toml version to 0.33.8 - Built new wheel with indentation fix from previous commit
- Fixed IndentationError at line 686 and subsequent lines - All code inside 'async with ClientSession' block now properly indented - Bumped version to 0.33.9 with complete fix
- Fresh build with properly indented ClientSession block - All indentation issues resolved in _customer.py
…er.py - Fixed indentation at line 801 in call_mcp_tool_customer function - Both _list_server_tools and call_mcp_tool_customer now properly indented - Bumped version to 0.33.11
- Fixed indentation at lines 284 and 398 in _lob.py - All ClientSession blocks now properly indented in both _customer.py and _lob.py - Ran full Python compilation check - all files pass - Bumped version to 0.33.12
- Removed intermediate wheel versions 0.33.7 through 0.33.12 - Latest version 0.33.13 is the current release
- File was added in fork but never used or imported - Does not exist in SAP upstream repository - Removed to align with upstream and avoid introducing unauthorized changes - Bumped version to 0.33.14
- Tests were removed in commit c75ccdf but function was restored - Added TestGetIasClientIdLob test class with 4 test methods - All tests pass: returns client ID, handles missing destination, handles missing property, handles missing env var - Added missing ConsumptionOptions import - Bumped version to 0.33.15
- Removed result is None check from call_mcp_tool_lob() - Removed result.isError check from call_mcp_tool_lob() - Removed result is None check from call_mcp_tool_customer() - Removed result.isError check from call_mcp_tool_customer() - These defensive checks were added in fork but don't exist in SAP upstream - Aligned with SAP upstream simple implementation - Bumped version to 0.33.16
…tream) - Removed ord_id parameter from AgentGatewayClient.list_mcp_tools() - Parameter was added in fork but doesn't exist in SAP upstream - Other filter logic is already in the works upstream - Breaks interface compatibility with SAP upstream - Removed from method signature, docstring, and both call sites - All tests pass (9 tests in TestListMcpTools) - Bumped version to 0.33.17
…P upstream) - Changed tool parameter from 'MCPTool | str' to just 'MCPTool' - Removed ord_id parameter from call_mcp_tool() - Removed _resolve_tool_by_name() helper method - Removed all isinstance(tool, str) resolution logic from customer/transparent/LoB flows - Simplified docstring: removed tool-by-name example and ord_id docs - Interface now matches SAP upstream exactly - All tests pass (10 tests in TestCallMcpTool) - Bumped version to 0.33.18
…date related tests
… integration dependencies
…tial loading and tests
Remove _telemetry_source=Module.AGENTGATEWAY from create_destination_client calls in _fetch_auth_token and get_ias_client_id_lob, and drop the now-unused Module import.
Contributor
Author
SDK Module Review
Findings (1)
1 finding(s): 1 posted as inline comment(s) on the affected lines, 0 not tied to a code line (listed above). Generated by sdk-review-skill · v1 |
… PT-08) - Extract _TOKEN_FIELD_CLIENT_ID and _TOKEN_FIELD_ACCESS_TOKEN constants in _customer.py - Extract _LOG_TRANSPARENT_MODE constant in agw_client.py - Add `from e` to raise in token request error handlers - Add type annotations and justify ImportError suppression in _telemetry_compat.py
…t and bump version - Use _TOKEN_FIELD_CLIENT_ID in required_vars and required_fields mappings - Bump version to 0.35.1
tiagoek
changed the title
tiagoek
commented
Jul 15, 2026
| _GRANT_TYPE_JWT_BEARER = "urn:ietf:params:oauth:grant-type:jwt-bearer" | ||
|
|
||
| # Token response field keys | ||
| _TOKEN_FIELD_CLIENT_ID = "client_id" |
Contributor
Author
There was a problem hiding this comment.
Fixed — the flag points at the constant declaration line itself (). The other two occurrences (lines 154, 207) have been replaced with in the latest commit. No remaining bare literals.
tiagoek
commented
Jul 15, 2026
| try: | ||
| from sap_cloud_sdk.core.telemetry import Module, Operation, record_metrics | ||
| TELEMETRY_AVAILABLE = True | ||
| except ImportError: |
Contributor
Author
There was a problem hiding this comment.
Intentional suppression — this is an optional-dependency compatibility shim. When sap_cloud_sdk.core.telemetry is not installed (e.g. lightweight Kyma deployments), the ImportError is expected and the except block provides no-op implementations so callers don't need to guard against missing telemetry. Re-raising would break the fallback contract. Comment added in the file to justify: # Telemetry packages not installed — provide no-op implementations.
tiagoek
commented
Jul 15, 2026
| try: | ||
| from sap_cloud_sdk.core.telemetry import Module, Operation, record_metrics | ||
| TELEMETRY_AVAILABLE = True | ||
| except ImportError: |
Contributor
Author
There was a problem hiding this comment.
[FLAG] PY-EL-02
except block does not end with raise — swallowing? Add re-raise or comment justifying suppression
Contributor
Author
|
Closing — superseded by a cleaner branch targeting Sebastian's branch directly with only the telemetry removal. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Requested by @smahr to remove telemetry source attribution from
_lob.pyso that internal destination calls made by the LoB auth helpers are not counted as cross-module traffic fromAGENTGATEWAYin observability dashboards.This branch includes @smahr's work from PR #220 (Transparent TLS + ServiceBinding Envelope Support) plus the targeted telemetry change.
What changes:
_telemetry_source=Module.AGENTGATEWAYfromcreate_destination_client()in_fetch_auth_tokenandget_ias_client_id_lobfrom sap_cloud_sdk.core.telemetry import Moduleimport0.35.1Why remove
_telemetry_sourcein_lob.py:Without
_telemetry_source,record_metricstreats these calls as user-facing (source=None) rather than internal SDK plumbing. The intent is that the auth/IAS resolution steps in the LoB flow should not show up asAGENTGATEWAY → DESTINATIONcross-module metrics.Scope intentionally limited to
_lob.py:_fragments.pyretains_telemetry_source=Module.AGENTGATEWAY— fragment discovery attribution is considered correct and out of scope here.Breaking Changes
Breaking Changes Details
The following public API parameters have been removed as part of @smahr's refactor (PR #220):
list_mcp_tools(..., ord_id=...)ord_idparameter removedcall_mcp_tool(tool: MCPTool|str, ...)toolno longer acceptsstr; must beMCPToolMCPToolobject fromlist_mcp_toolsdirectlyapp_tidparameterget_system_auth/get_user_auth/list_mcp_tools/call_mcp_toolTest plan
_fetch_auth_tokenstill resolves auth tokens correctlyget_ias_client_id_lobstill returns the client ID correctly