Native instrumentation with OpenTelemetry#329
Conversation
…into users/robrandao/otel-sample
There was a problem hiding this comment.
Pull request overview
This pull request introduces native OpenTelemetry instrumentation to the Microsoft Agents SDK, enabling comprehensive observability through traces, metrics, and logs. The changes add telemetry hooks at key points in the agent lifecycle including turn processing, adapter operations, and storage interactions, while also providing a test sample demonstrating the integration.
Changes:
- Added OpenTelemetry integration to core SDK with AgentTelemetry class for instrumentation
- Integrated telemetry tracking into agent turn processing, adapter operations, and storage methods
- Created test sample demonstrating OpenTelemetry configuration and usage with aiohttp
- Enhanced testing framework to support Activity template aliasing (from/from_property compatibility)
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 27 comments.
Show a summary per file
| File | Description |
|---|---|
| test_samples/otel/src/telemetry.py | OpenTelemetry configuration helper for test sample |
| test_samples/otel/src/start_server.py | Sample aiohttp server with telemetry integration |
| test_samples/otel/src/requirements.txt | Dependencies for OpenTelemetry sample |
| test_samples/otel/src/main.py | Entry point configuring telemetry before agent initialization |
| test_samples/otel/src/env.TEMPLATE | Environment configuration template with OTLP settings |
| test_samples/otel/src/agent_metric.py | Sample-specific metrics wrapper (has import name mismatch) |
| test_samples/otel/src/agent.py | Sample agent with manual telemetry instrumentation |
| libraries/microsoft-agents-hosting-core/setup.py | Added OpenTelemetry dependencies |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/observability/_agent_telemetry.py | Core telemetry infrastructure with context managers for instrumentation |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/observability/init.py | Exports AgentTelemetry and agent_telemetry singleton |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/agent_application.py | Wraps turn processing with telemetry |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/storage.py | Adds telemetry to base storage operations |
| libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/memory_storage.py | Adds telemetry to in-memory storage implementation |
| libraries/microsoft-agents-storage-cosmos/microsoft_agents/storage/cosmos/cosmos_db_storage.py | Minor whitespace change |
| libraries/microsoft-agents-hosting-fastapi/microsoft_agents/hosting/fastapi/cloud_adapter.py | Wraps adapter processing with telemetry |
| libraries/microsoft-agents-hosting-aiohttp/microsoft_agents/hosting/aiohttp/cloud_adapter.py | Wraps adapter processing with telemetry |
| dev/tests/sdk/observability/test_observability.py | Tests for telemetry functionality |
| dev/tests/scenarios/quickstart.py | Renamed init_app to init_agent |
| dev/tests/scenarios/init.py | Updated to use renamed init_agent function |
| dev/microsoft-agents-testing/tests/core/fluent/test_model_template.py | Added tests for from/from_property aliasing |
| dev/microsoft-agents-testing/microsoft_agents/testing/core/fluent/utils.py | Added from/from_property aliasing logic |
| dev/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py | Integrated aliasing into template classes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test_samples/otel/src/telemetry.py
Outdated
| from microsoft_agents.hosting.core import TurnContext | ||
|
|
There was a problem hiding this comment.
Unused import: TurnContext is imported but never used in this file. Consider removing it to keep imports clean.
| from microsoft_agents.hosting.core import TurnContext |
| from opentelemetry.sdk.trace import TracerProvider | ||
| from opentelemetry.sdk.trace.export import SimpleSpanProcessor, ConsoleSpanExporter | ||
|
|
||
|
|
There was a problem hiding this comment.
Unused imports: TracerProvider, SimpleSpanProcessor, and ConsoleSpanExporter are imported but never used in this file. Remove them to keep imports clean.
| from opentelemetry.sdk.trace import TracerProvider | |
| from opentelemetry.sdk.trace.export import SimpleSpanProcessor, ConsoleSpanExporter |
| def _ts() -> float: | ||
| """Helper function to get current timestamp in milliseconds""" | ||
| return datetime.now(timezone.utc).timestamp() * 1000 | ||
|
|
There was a problem hiding this comment.
Unused function: The _ts() helper function is defined but never used in this file. Consider removing it.
| def _ts() -> float: | |
| """Helper function to get current timestamp in milliseconds""" | |
| return datetime.now(timezone.utc).timestamp() * 1000 |
|
|
||
| def __init__(self, tracer: Tracer | None = None, meter: Meter | None = None): | ||
| if tracer is None: | ||
| tracer = trace.get_tracer("M365.agents", "1.0.0") |
There was a problem hiding this comment.
Inconsistent indentation: Line 33 has an extra space before 'tracer'. Should be a single space like line 35.
| tracer = trace.get_tracer("M365.agents", "1.0.0") | |
| tracer = trace.get_tracer("M365.agents", "1.0.0") |
| with self.tracer.start_as_current_span(span_name) as span: | ||
| attributes = self._extract_attributes_from_context(context) | ||
| span.set_attributes(attributes) | ||
| # span.add_event(f"{span_name} started", attributes) |
There was a problem hiding this comment.
Commented-out code: Line 97 contains a commented-out span.add_event() call. Either remove it if it's not needed or uncomment it if it should be active.
| # span.add_event(f"{span_name} started", attributes) |
| async def entry_point(req: Request) -> Response: | ||
|
|
||
| logger.info("Request received at /api/messages endpoint.") | ||
| text = await req.text() |
There was a problem hiding this comment.
Unused variable: The 'text' variable on line 23 is assigned but never used. Remove it if it's not needed.
| text = await req.text() |
| APP["adapter"] = agent_application.adapter | ||
|
|
||
| try: | ||
| run_app(APP, host="localhost", port=environ.get("PORT", 3978)) |
There was a problem hiding this comment.
Type mismatch: environ.get("PORT", 3978) returns a string or int, but the port parameter expects an int. The default value 3978 is an int, but if PORT is set in the environment it will be a string. Convert to int: port=int(environ.get("PORT", 3978)).
| run_app(APP, host="localhost", port=environ.get("PORT", 3978)) | |
| run_app(APP, host="localhost", port=int(environ.get("PORT", 3978))) |
| self.tracer = trace.get_tracer("A365.AgentFramework") | ||
| self.meter = metrics.get_meter("A365.AgentFramework", "1.0.0") |
There was a problem hiding this comment.
Inconsistent naming: The test sample uses "A365.AgentFramework" for the tracer/meter name (line 28-29), while the core library uses "M365.agents" (see libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/observability/_agent_telemetry.py line 33). Consider using consistent naming across the codebase for better observability and to avoid confusion.
| self.tracer = trace.get_tracer("A365.AgentFramework") | |
| self.meter = metrics.get_meter("A365.AgentFramework", "1.0.0") | |
| self.tracer = trace.get_tracer("M365.agents") | |
| self.meter = metrics.get_meter("M365.agents", "1.0.0") |
| success_callback(span, duration) | ||
| else: | ||
|
|
||
| if failure_callback: | ||
| failure_callback(span, exception) |
There was a problem hiding this comment.
Missing error handling for callbacks: If success_callback or failure_callback raise an exception in lines 137-142, it will propagate and potentially mask the original exception. Consider wrapping callback invocations in try-except blocks to ensure telemetry errors don't break application logic.
| success_callback(span, duration) | |
| else: | |
| if failure_callback: | |
| failure_callback(span, exception) | |
| try: | |
| success_callback(span, duration) | |
| except Exception as callback_exc: | |
| # Ensure telemetry callback failures do not affect application logic | |
| span.record_exception(callback_exc) | |
| else: | |
| if failure_callback: | |
| try: | |
| failure_callback(span, exception) | |
| except Exception as callback_exc: | |
| # Ensure telemetry callback failures do not mask the original exception | |
| span.record_exception(callback_exc) |
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST=".*" | ||
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE=".*" | ||
|
|
||
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_CLIENT_REQUEST=".*" | ||
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_CLIENT_RESPONSE=".*" No newline at end of file |
There was a problem hiding this comment.
Using OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_* with value ".*" causes OpenTelemetry to capture and export all HTTP request and response headers, including Authorization, Cookie, and other secret-bearing headers. Anyone with access to the telemetry backend or pipeline could then retrieve credentials or other sensitive data from these exported headers. Limit these variables to a specific allowlist of non-sensitive headers and explicitly exclude authentication and other secret-related headers from capture.
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST=".*" | |
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE=".*" | |
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_CLIENT_REQUEST=".*" | |
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_CLIENT_RESPONSE=".*" | |
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_REQUEST="user-agent,x-request-id,x-correlation-id" | |
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_SERVER_RESPONSE="content-type,content-length,x-request-id,x-correlation-id" | |
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_CLIENT_REQUEST="user-agent,x-request-id,x-correlation-id" | |
| OTEL_INSTRUMENTATION_HTTP_CAPTURE_HEADERS_CLIENT_RESPONSE="content-type,content-length,x-request-id,x-correlation-id" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 24 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tracer_provider.add_span_processor(SimpleSpanProcessor(exporter)) | ||
| trace.set_tracer_provider(tracer_provider) | ||
|
|
||
| meter_provider = MeterProvider([metric_reader]) |
There was a problem hiding this comment.
MeterProvider([metric_reader]) is likely using the wrong constructor signature (the metric readers are typically passed via the metric_readers= keyword). As written, this may raise a TypeError or silently misconfigure metrics; initialize MeterProvider with the in-memory reader using the supported parameter name for your OpenTelemetry SDK version.
| meter_provider = MeterProvider([metric_reader]) | |
| meter_provider = MeterProvider(metric_readers=[metric_reader]) |
test_samples/otel/src/telemetry.py
Outdated
| from microsoft_agents.hosting.core import TurnContext | ||
|
|
||
| import aiohttp | ||
| from opentelemetry import metrics, trace | ||
| from opentelemetry.trace import Span | ||
| from opentelemetry._logs import set_logger_provider |
There was a problem hiding this comment.
configure_telemetry is used by an aiohttp sample, but the docstring says "FastAPI application" and TurnContext is imported but unused. Update the docstring to match the actual usage and remove unused imports to avoid confusion.
|
|
||
| def __init__(self, tracer: Tracer | None = None, meter: Meter | None = None): | ||
| if tracer is None: | ||
| tracer = trace.get_tracer("M365.agents", "1.0.0") |
There was a problem hiding this comment.
There is inconsistent indentation under if tracer is None: (tracer = ... is indented one extra space). This will raise an IndentationError when importing the module. Fix indentation to be consistent with the surrounding block.
| tracer = trace.get_tracer("M365.agents", "1.0.0") | |
| tracer = trace.get_tracer("M365.agents", "1.0.0") |
| if key == "": | ||
| raise ValueError(str(storage_errors.CosmosDbKeyCannotBeEmpty)) | ||
|
|
||
There was a problem hiding this comment.
Trailing whitespace was introduced on this blank line. Please remove it to keep diffs clean and avoid failing whitespace checks.
| if not target_cls: | ||
| result[key] = self._memory[key] | ||
| else: | ||
| try: | ||
| result[key] = target_cls.from_json_to_store_item( | ||
| self._memory[key] | ||
| ) | ||
| except AttributeError as error: | ||
| raise TypeError( | ||
| f"MemoryStorage.read(): could not deserialize in-memory item into {target_cls} class. Error: {error}" | ||
| ) |
There was a problem hiding this comment.
target_cls is validated as required at the top of read() (raising if falsy), so the inner if not target_cls: branch is dead code. Consider removing the redundant branch and simplifying the control flow in this loop.
| if not target_cls: | |
| result[key] = self._memory[key] | |
| else: | |
| try: | |
| result[key] = target_cls.from_json_to_store_item( | |
| self._memory[key] | |
| ) | |
| except AttributeError as error: | |
| raise TypeError( | |
| f"MemoryStorage.read(): could not deserialize in-memory item into {target_cls} class. Error: {error}" | |
| ) | |
| try: | |
| result[key] = target_cls.from_json_to_store_item( | |
| self._memory[key] | |
| ) | |
| except AttributeError as error: | |
| raise TypeError( | |
| f"MemoryStorage.read(): could not deserialize in-memory item into {target_cls} class. Error: {error}" | |
| ) |
| ) | ||
| from aiohttp.web import Request, Response, Application, run_app, json_response | ||
|
|
||
| from .agent_metrics import agent_metrics |
There was a problem hiding this comment.
from .agent_metrics import agent_metrics will fail at runtime because the file in this sample is agent_metric.py (singular). Rename the module to match, or update the import (and any other references) so the module name is consistent.
| from .agent_metrics import agent_metrics | |
| from .agent_metric import agent_metrics |
test_samples/otel/src/agent.py
Outdated
| from microsoft_agents.authentication.msal import MsalConnectionManager | ||
| from microsoft_agents.activity import load_configuration_from_env | ||
|
|
||
| from .agent_metrics import agent_metrics |
There was a problem hiding this comment.
This import points to .agent_metrics, but the sample file is named agent_metric.py. As-is, importing AGENT_APP will raise ModuleNotFoundError. Align the filename and imports (either rename the file or change the import here and in start_server.py).
| from .agent_metrics import agent_metrics | |
| from .agent_metric import agent_metrics |
| if success: | ||
| span.set_status(trace.Status(trace.StatusCode.OK)) | ||
| if success_callback: | ||
| success_callback(span, duration) | ||
| else: | ||
|
|
||
| if failure_callback: | ||
| failure_callback(span, exception) | ||
|
|
||
| span.set_status(trace.Status(trace.StatusCode.ERROR)) | ||
| raise exception from None # re-raise to ensure it's not swallowed |
There was a problem hiding this comment.
trace.Status / trace.StatusCode are not part of the opentelemetry.trace module API; this will raise AttributeError at runtime. Import Status and StatusCode from opentelemetry.trace (or use the correct API) and set status via those types.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 158 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py:15
- There is an unused import (
from email.mime import base) introduced here. It increases import-time overhead and may confuse readers; please remove it.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test_samples/otel/src/agent.py
Outdated
|
|
||
| from .agent_metrics import agent_metrics | ||
|
|
There was a problem hiding this comment.
agent_metrics is imported from .agent_metrics, but the only module added in this sample is agent_metric.py. This import will fail at runtime; rename the module or fix the import to match the actual filename.
| "isodate>=0.6.1", | ||
| "azure-core>=1.30.0", | ||
| "python-dotenv>=1.1.1", | ||
| "opentelemetry-api>=1.17.0", # TODO -> verify this before commit | ||
| "opentelemetry-sdk>=1.17.0", |
There was a problem hiding this comment.
microsoft_agents.hosting.core.observability.configure_telemetry imports OTLP exporters (opentelemetry-exporter-otlp), but install_requires only adds opentelemetry-api/opentelemetry-sdk. As-is, importing the observability package can raise ModuleNotFoundError in environments that don't have the exporter installed. Add the missing runtime dependency (or move OTLP exporter support behind an optional extra and avoid importing it from observability.__init__).
| tracer_provider = TracerProvider() | ||
| tracer_provider.add_span_processor(SimpleSpanProcessor(exporter)) | ||
| trace.set_tracer_provider(tracer_provider) | ||
|
|
||
| meter_provider = MeterProvider([metric_reader]) | ||
|
|
||
| metrics.set_meter_provider(meter_provider) | ||
|
|
There was a problem hiding this comment.
MeterProvider([metric_reader]) is likely passing the InMemoryMetricReader list as the resource positional arg rather than registering it as a metric reader. Construct the provider using the explicit metric_readers=[...] (or the correct keyword for your targeted OTel version) so the metric reader is actually attached.
| raise ValueError(str(storage_errors.CosmosDbKeyCannotBeEmpty)) | ||
|
|
||
| escaped_key: str = self._sanitize(key) |
There was a problem hiding this comment.
This blank line contains trailing whitespace. Please remove it (and run the formatter) to avoid noisy diffs and style-check failures.
|
|
||
| from .agent_metrics import agent_metrics | ||
|
|
There was a problem hiding this comment.
agent_metrics is imported from .agent_metrics, but this sample adds agent_metric.py (singular). This import will fail at runtime unless the module name is aligned.
| # Add logging handler | ||
| handler = LoggingHandler(level=logging.NOTSET, logger_provider=logger_provider) | ||
| logging.getLogger().addHandler(handler) | ||
|
|
There was a problem hiding this comment.
configure_telemetry() always adds a new LoggingHandler to the root logger. If this function is called more than once (e.g., in tests, reloads, or multiple app instances), logs will be duplicated. Consider making the configuration idempotent (check for an existing handler) or scoping the handler to a named logger.
| from ._agent_telemetry import AgentTelemetry, agent_telemetry | ||
| from .configure_telemetry import configure_telemetry | ||
| from .constants import ( | ||
| SERVICE_NAME, | ||
| SERVICE_VERSION, | ||
| RESOURCE | ||
| ) |
There was a problem hiding this comment.
Importing microsoft_agents.hosting.core.observability will eagerly import configure_telemetry, which in turn imports OTLP exporter modules. Unless exporter dependencies are guaranteed to be installed, this makes observability imports fragile. Consider lazily importing configure_telemetry (or guarding it with a helpful error) so agent_telemetry remains usable without optional exporter packages.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 164 changed files in this pull request and generated 16 comments.
Comments suppressed due to low confidence (1)
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py:14
- Unused import
from email.mime import baselooks accidental and will trigger lint/static-check failures. Please remove it (or use it if it was intended).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with agent_telemetry.adapter_process_operation(): | ||
| # Adapt request to protocol | ||
| adapted_request = FastApiRequestAdapter(request) |
There was a problem hiding this comment.
agent_telemetry currently has invoke_adapter_process_op() but no adapter_process_operation(). As written, this will raise AttributeError when processing requests; align the method name or add a backwards-compatible alias on AgentTelemetry.
| with agent_telemetry.storage_operation("read"): | ||
| pass |
There was a problem hiding this comment.
agent_telemetry currently has invoke_storage_op(operation) but no storage_operation(operation). This test will fail with AttributeError unless the API names are aligned.
| # Get OTLP endpoint from environment or use default for standalone dashboard | ||
| otlp_endpoint = os.getenv("OTEL_EXPORTER_OTLP_ENDPOINT", "http://localhost:4317") | ||
|
|
||
| # Configure Tracing | ||
| trace_provider = TracerProvider(resource=RESOURCE) | ||
| trace_provider.add_span_processor( | ||
| BatchSpanProcessor(OTLPSpanExporter(endpoint=otlp_endpoint)) | ||
| ) |
There was a problem hiding this comment.
configure_telemetry() uses the OTLP HTTP exporters (opentelemetry.exporter.otlp.proto.http.*) but defaults OTEL_EXPORTER_OTLP_ENDPOINT to http://localhost:4317, which is the OTLP gRPC default port. This mismatch will fail to export by default; either switch to the gRPC exporters or change the default endpoint/port to the OTLP HTTP default (commonly http://localhost:4318, with the correct per-signal paths as required).
| "opentelemetry-api>=1.17.0", # TODO -> verify this before commit | ||
| "opentelemetry-sdk>=1.17.0", | ||
| "opentelemetry-exporter-otlp-proto-http>=1.17.0", |
There was a problem hiding this comment.
The new OpenTelemetry dependencies include an inline # TODO -> verify this before commit comment. Shipping TODOs in install_requires is easy to miss and makes releases ambiguous; please either remove the TODO or resolve it (and consider whether these should be optional extras rather than unconditional core deps).
| with agent_telemetry.adapter_process_operation(): | ||
| # Adapt request to protocol | ||
| adapted_request = AiohttpRequestAdapter(request) |
There was a problem hiding this comment.
agent_telemetry currently has invoke_adapter_process_op() but no adapter_process_operation(). As written, this will raise AttributeError when processing requests; align the method name or add a backwards-compatible alias on AgentTelemetry.
| with agent_telemetry.storage_operation("read"): | ||
| items: list[tuple[Union[str, None], Union[StoreItemT, None]]] = await gather( | ||
| *[self._read_item(key, target_cls=target_cls, **kwargs) for key in keys] | ||
| ) | ||
| return {key: value for key, value in items if key is not None} |
There was a problem hiding this comment.
agent_telemetry does not expose storage_operation() (the current AgentTelemetry API uses invoke_storage_op(operation)). These wrappers will currently raise AttributeError for all storage reads; align the method name or add an alias on AgentTelemetry.
| with agent_telemetry.storage_operation("delete"): | ||
| await gather(*[self._delete_item(key) for key in keys]) |
There was a problem hiding this comment.
Same issue as read(): agent_telemetry does not expose storage_operation() (only invoke_storage_op(...)). This will raise AttributeError on deletes unless the API names are aligned.
| with agent_telemetry.storage_operation("delete"): | ||
| for key in keys: | ||
| if key == "": | ||
| raise ValueError("MemoryStorage.delete(): key cannot be empty") |
There was a problem hiding this comment.
Same issue as read(): agent_telemetry does not expose storage_operation() (only invoke_storage_op(...)). This will raise AttributeError on deletes unless the API names are aligned.
| with agent_telemetry.agent_turn_operation(context): | ||
| pass |
There was a problem hiding this comment.
agent_telemetry currently has invoke_agent_turn_op(context) but no agent_turn_operation(context). This test will fail with AttributeError unless the API names are aligned.
| with agent_telemetry.adapter_process_operation(): | ||
| pass |
There was a problem hiding this comment.
agent_telemetry currently has invoke_adapter_process_op() but no adapter_process_operation(). This test will fail with AttributeError unless the API names are aligned.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 175 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (20)
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/channel_service_adapter.py:1
start_span_adapter_send_activities(activities)is entered before validatingactivitiesis non-null and non-empty. The span helper readslen(activities)and may accessactivities[0], soactivities is Noneorlen == 0will raise before the intended error handling. Move the span context manager below these validations, or makestart_span_adapter_send_activitiesresilient toNone/empty inputs.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/connector/client/connector_client.py:1- These span context managers are called with arguments that don't match the definitions in
hosting.core.telemetry.spans. For example,start_span_connector_reply_to_activityexpects(conversation_id, activity_id)but the code passesbody. Similarly,start_span_connector_update_activityexpects(conversation_id, activity_id)but receivesbody. Update the calls to pass the correct identifiers (and considerconstants.UNKNOWNwhen IDs are missing) so connector operations can be traced without raisingTypeError.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/connector/client/connector_client.py:1 - These span context managers are called with arguments that don't match the definitions in
hosting.core.telemetry.spans. For example,start_span_connector_reply_to_activityexpects(conversation_id, activity_id)but the code passesbody. Similarly,start_span_connector_update_activityexpects(conversation_id, activity_id)but receivesbody. Update the calls to pass the correct identifiers (and considerconstants.UNKNOWNwhen IDs are missing) so connector operations can be traced without raisingTypeError.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/connector/client/connector_client.py:1 - These span context managers are called with arguments that don't match the definitions in
hosting.core.telemetry.spans. For example,start_span_connector_reply_to_activityexpects(conversation_id, activity_id)but the code passesbody. Similarly,start_span_connector_update_activityexpects(conversation_id, activity_id)but receivesbody. Update the calls to pass the correct identifiers (and considerconstants.UNKNOWNwhen IDs are missing) so connector operations can be traced without raisingTypeError.
libraries/microsoft-agents-hosting-aiohttp/microsoft_agents/hosting/aiohttp/cloud_adapter.py:1 agents_telemetry.instrument_adapter_process()is not defined on_AgentsTelemetryin the new telemetry implementation, so this will raiseAttributeErrorat runtime. Either addinstrument_adapter_process()toagents_telemetry(and implement it), or switch to the existing span helper (e.g.,spans.start_span_adapter_process(...)) with the correct inputs.
libraries/microsoft-agents-hosting-fastapi/microsoft_agents/hosting/fastapi/cloud_adapter.py:1- Same issue as aiohttp:
_AgentsTelemetrydoesn't defineinstrument_adapter_process(), so this will fail at runtime. Use an existing span helper fromtelemetry.spansor implementinstrument_adapter_process()in the core telemetry API.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/agent_application.py:1 - The span helper names / call signatures don’t match the newly added
telemetry.spansmodule:start_span_app_on_turnis defined to take aTurnContextProtocol, but this call passescontext.activity. Alsostart_span_app_file_downloadsandstart_span_app_router_handlerdon’t exist (the module definesstart_span_app_download_filesandstart_span_app_route_handler). Align the call sites with the actual helpers and pass the correct object types to avoid runtime errors.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/agent_application.py:1 - The span helper names / call signatures don’t match the newly added
telemetry.spansmodule:start_span_app_on_turnis defined to take aTurnContextProtocol, but this call passescontext.activity. Alsostart_span_app_file_downloadsandstart_span_app_router_handlerdon’t exist (the module definesstart_span_app_download_filesandstart_span_app_route_handler). Align the call sites with the actual helpers and pass the correct object types to avoid runtime errors.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/agent_application.py:1 - The span helper names / call signatures don’t match the newly added
telemetry.spansmodule:start_span_app_on_turnis defined to take aTurnContextProtocol, but this call passescontext.activity. Alsostart_span_app_file_downloadsandstart_span_app_router_handlerdon’t exist (the module definesstart_span_app_download_filesandstart_span_app_route_handler). Align the call sites with the actual helpers and pass the correct object types to avoid runtime errors.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/channel_service_adapter.py:1 start_span_adapter_continue_continue_conversationdoes not exist inhosting.core.telemetry.spans(the available helper isstart_span_adapter_continue_conversation). This will raiseAttributeErrorat runtime; rename the call to the correct context manager.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/telemetry/constants.py:1- Typo in constant name:
UPDLOADshould beUPLOADfor clarity and consistency.
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/telemetry/spans.py:1 spanis referenced but never defined because thewithstatement does not bindas span. This will raiseNameErrorwhen the context manager runs. Bind the span (e.g.,... as span) or avoid usingspanhere.
libraries/microsoft-agents-authentication-msal/microsoft_agents/authentication/msal/msal_auth.py:1- The telemetry helper name looks incorrect: the added telemetry module defines
start_span_auth_get_agentic_user_token(...), but this call usesstart_span_get_agentic_user_token(...). This will raiseAttributeErrorat runtime unless there’s an alias elsewhere. Update to the correct function name exported bymicrosoft_agents.authentication.msal.telemetry.spans.
test_samples/otel/src/start_server.py:1 - The sample imports
.agent_metrics, but the added file isagent_metric.py(singular). This will fail withModuleNotFoundErrorwhen running the sample. Rename the module or fix the import to match the actual filename.
test_samples/otel/src/agent.py:1 - Same module mismatch as
start_server.py:.agent_metricsdoesn’t match the added fileagent_metric.py. Fix the import or filename so the sample can run.
dev/testing/python-sdk-tests/sdk/hosting-core/telemetry/test_telemetry.py:1 - The test asserts span names/constants that don't exist or don't match the new telemetry constants:
\"agent turn\",constants.SPAN_APP_ON_TURN, andconstants.SPANS_ADAPTER_PROCESSare not defined intelemetry.constantsas added in this PR. Update assertions to use the correct constants (e.g.,constants.SPAN_APP_RUN,constants.SPAN_ADAPTER_PROCESS) and expected emitted span names so the integration tests reflect the actual instrumentation.
dev/testing/python-sdk-tests/sdk/hosting-core/telemetry/test_telemetry.py:1 - The test asserts span names/constants that don't exist or don't match the new telemetry constants:
\"agent turn\",constants.SPAN_APP_ON_TURN, andconstants.SPANS_ADAPTER_PROCESSare not defined intelemetry.constantsas added in this PR. Update assertions to use the correct constants (e.g.,constants.SPAN_APP_RUN,constants.SPAN_ADAPTER_PROCESS) and expected emitted span names so the integration tests reflect the actual instrumentation.
dev/testing/python-sdk-tests/sdk/hosting-core/telemetry/test_telemetry.py:1 - The test asserts span names/constants that don't exist or don't match the new telemetry constants:
\"agent turn\",constants.SPAN_APP_ON_TURN, andconstants.SPANS_ADAPTER_PROCESSare not defined intelemetry.constantsas added in this PR. Update assertions to use the correct constants (e.g.,constants.SPAN_APP_RUN,constants.SPAN_ADAPTER_PROCESS) and expected emitted span names so the integration tests reflect the actual instrumentation.
dev/testing/python-sdk-tests/sdk/hosting-core/telemetry/test_telemetry.py:1 - The test asserts span names/constants that don't exist or don't match the new telemetry constants:
\"agent turn\",constants.SPAN_APP_ON_TURN, andconstants.SPANS_ADAPTER_PROCESSare not defined intelemetry.constantsas added in this PR. Update assertions to use the correct constants (e.g.,constants.SPAN_APP_RUN,constants.SPAN_ADAPTER_PROCESS) and expected emitted span names so the integration tests reflect the actual instrumentation.
libraries/microsoft-agents-hosting-core/setup.py:1 - A TODO comment inside
install_requiresis easy to ship unintentionally and doesn’t provide actionable guidance to downstream consumers. Please remove the TODO before merging, or move the verification note to a tracking issue/PR description.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 178 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py:89
- ModelTemplate.with_defaults() normalizes
defaultsvia rename_from_property(), but it passes**kwargsthrough unmodified. This means callers using the supported alias keys in kwargs (e.g.,with_defaults(**{'from.id': ...})) won’t be normalized and will produce inconsistent templates (and likely fail the new alias tests). Normalize/flatten kwargs the same way asdefaultsbefore calling set_defaults().
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py:147 - ActivityTemplate.with_defaults() has the same issue as ModelTemplate.with_defaults(): it normalizes only the
defaultsdict but not**kwargs. Calls likeActivityTemplate(...).with_defaults(**{'from.id': ...})will not be normalized tofrom_property.*. Normalize kwargs (e.g., via flatten_model_data/rename_from_property) before calling set_defaults().
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py:14 from email.mime import baseis unused and looks accidental here. Please remove it to avoid lint failures / confusion.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from microsoft_agents.hosting.core import ChannelServiceClientFactoryBase | ||
| from microsoft_agents.hosting.core.telemetry import agents_telemetry | ||
|
|
There was a problem hiding this comment.
agents_telemetry is imported but never used in this module. Remove the import to avoid unused-import lint failures.
| from microsoft_agents.hosting.core import ChannelServiceClientFactoryBase | ||
| from microsoft_agents.hosting.core.telemetry import spans | ||
|
|
There was a problem hiding this comment.
spans is imported but not used anywhere in this module. Remove the import (or add the intended instrumentation) to avoid unused-import lint failures.
| def start_span_auth_get_agentic_user_token(agentic_instance_id: str, agentic_user_id: str, scopes: list[str]) -> Iterator[None]: | ||
| with agents_telemetry.start_as_current_span(constants.SPAN_AUTH_GET_AGENTIC_USER_TOKEN): | ||
| span.set_attributes({ | ||
| common_constants.ATTR_AGENTIC_INSTANCE_ID: agentic_instance_id, | ||
| common_constants.ATTR_AGENTIC_USER_ID: agentic_user_id, | ||
| common_constants.ATTR_AUTH_SCOPES: scopes, | ||
| }) |
There was a problem hiding this comment.
In start_span_auth_get_agentic_user_token(), the span context manager doesn’t bind as span, but the body calls span.set_attributes(...), which will raise NameError at runtime. Bind the span (... as span) and also ensure the auth scopes attribute is set to a supported scalar type (e.g., use _format_scopes(scopes) like the other helpers).
| SPAN_CONNECTOR_UPDLOAD_ATTACHMENT = "agents.connector.uploadAttachment" | ||
| SPAN_CONNECTOR_GET_ATTACHMENT = "agents.connector.getAttachment" |
There was a problem hiding this comment.
The constant name/value SPAN_CONNECTOR_UPDLOAD_ATTACHMENT appears to have a typo (“UPDLOAD”). This will propagate into emitted span names (see usages in telemetry/spans.py), making dashboards/queries inconsistent. Rename to SPAN_CONNECTOR_UPLOAD_ATTACHMENT (and update references) while this is still new.
| "python-dotenv>=1.1.1", | ||
| "opentelemetry-api>=1.17.0", # TODO -> verify this before commit | ||
| "opentelemetry-sdk>=1.17.0", | ||
| "opentelemetry-exporter-otlp-proto-http>=1.17.0", |
There was a problem hiding this comment.
Please remove the inline "TODO -> verify this before commit" from install_requires. If the versions need verification, do it before merging (or add a tracking issue); leaving TODOs in published dependency metadata makes releases harder to manage.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 179 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py:15
- Unused import
from email.mime import basewas added and is not referenced. This will trip linters and should be removed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| from microsoft_agents.hosting.core import ChannelServiceClientFactoryBase | ||
| from microsoft_agents.hosting.core.telemetry import spans | ||
|
|
||
| from .agent_http_adapter import AgentHttpAdapter |
There was a problem hiding this comment.
Unused import: spans is imported but not used in this file. If you intended to wrap process() (or similar) in spans, add that usage; otherwise remove the import to avoid unused-import tooling failures.
| # | ||
| # NOTE: this module should not be auto-loaded from __init__.py in order to avoid | ||
|
|
||
| from ._agents_telemetry import ( |
There was a problem hiding this comment.
This module-level comment ends mid-sentence (NOTE: this module should not be auto-loaded...) which makes it unclear what the intended guidance is. Either complete the note (what are we avoiding?) or remove it to prevent confusion for future maintainers.
| from microsoft_agents.hosting.core import ChannelServiceClientFactoryBase | ||
| from microsoft_agents.hosting.core.telemetry import agents_telemetry | ||
|
|
There was a problem hiding this comment.
Unused import: agents_telemetry is imported but never referenced in this module. If initialization side-effects are intended, it would be clearer to add a comment; otherwise remove the import to avoid lint failures.
| with agents_telemetry.start_as_current_span( | ||
| constants.SPAN_AUTH_GET_AGENTIC_USER_TOKEN | ||
| ): | ||
| span.set_attributes( | ||
| { | ||
| common_constants.ATTR_AGENTIC_INSTANCE_ID: agentic_instance_id, | ||
| common_constants.ATTR_AGENTIC_USER_ID: agentic_user_id, | ||
| common_constants.ATTR_AUTH_SCOPES: scopes, | ||
| } | ||
| ) |
There was a problem hiding this comment.
start_span_auth_get_agentic_user_token enters agents_telemetry.start_as_current_span(...) without as span, but then calls span.set_attributes(...), which will raise NameError at runtime. Bind the span (... as span) and consider formatting scopes via _format_scopes(scopes) for consistency with the other auth span helpers.
| tracer_provider = TracerProvider() | ||
| tracer_provider.add_span_processor(SimpleSpanProcessor(exporter)) | ||
| trace.set_tracer_provider(tracer_provider) | ||
|
|
||
| meter_provider = MeterProvider([metric_reader]) |
There was a problem hiding this comment.
This fixture sets global OpenTelemetry providers (trace.set_tracer_provider / metrics.set_meter_provider). OTel provider setters are effectively single-assignment, so multiple test modules doing this can interfere with each other and make in-memory exporter capture non-deterministic. Consider a shared session-scoped provider fixture and/or resetting OTel globals between suites.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 82 out of 213 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/transport/aiohttp_sender.py:75
AiohttpSender.send()raisesClientErroronresponse.status >= 300and then immediately re-raises in theexceptwhen a response exists, which prevents creating/recording anExchangefor non-2xx responses. SinceExchange.from_request()now supportsstatus>=300to capture error bodies, consider removing the explicit raise/re-raise and always build anExchangeso transcripts include failure details.
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py:15- Unused import:
from email.mime import baseis not referenced anywhere in this module and will trigger linting/type-check noise. Remove the import.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| with agents_telemetry.start_as_current_span( | ||
| constants.SPAN_AUTH_GET_AGENTIC_USER_TOKEN | ||
| ): | ||
| span.set_attributes( | ||
| { | ||
| common_constants.ATTR_AGENTIC_INSTANCE_ID: agentic_instance_id, | ||
| common_constants.ATTR_AGENTIC_USER_ID: agentic_user_id, | ||
| common_constants.ATTR_AUTH_SCOPES: scopes, | ||
| } | ||
| ) | ||
| yield |
| if not keys: | ||
| raise ValueError("Storage.read(): Keys are required when reading.") | ||
| if not target_cls: | ||
| raise ValueError("Storage.read(): target_cls cannot be None.") | ||
|
|
||
| result: dict[str, StoreItem] = {} | ||
| with self._lock: | ||
| for key in keys: | ||
| if key == "": | ||
| raise ValueError("MemoryStorage.read(): key cannot be empty") | ||
| if key in self._memory: | ||
| if not target_cls: | ||
| result[key] = self._memory[key] | ||
| else: | ||
| try: | ||
| result[key] = target_cls.from_json_to_store_item( | ||
| self._memory[key] | ||
| ) | ||
| except AttributeError as error: | ||
| raise TypeError( | ||
| f"MemoryStorage.read(): could not deserialize in-memory item into {target_cls} class. Error: {error}" | ||
| ) | ||
| return result | ||
|
|
||
| with spans.start_span_storage_read(len(keys)): | ||
| result: dict[str, StoreItem] = {} | ||
| with self._lock: | ||
| for key in keys: | ||
| if key == "": | ||
| raise ValueError("MemoryStorage.read(): key cannot be empty") | ||
| if key in self._memory: | ||
| if not target_cls: | ||
| result[key] = self._memory[key] | ||
| else: | ||
| try: |
| try: | ||
| run_app(APP, host="localhost", port=environ.get("PORT", 3978)) | ||
| except Exception as error: | ||
| raise error |
| SPAN_CONNECTOR_GET_CONVERSATION_MEMBERS = "agents.connector.getConversationMembers" | ||
| SPAN_CONNECTOR_UPDLOAD_ATTACHMENT = "agents.connector.uploadAttachment" | ||
| SPAN_CONNECTOR_GET_ATTACHMENT = "agents.connector.getAttachment" |
| def start_span_connector_upload_attachment(conversation_id: str) -> Iterator[None]: | ||
| with _start_span_connector_op( | ||
| constants.SPAN_CONNECTOR_UPDLOAD_ATTACHMENT, conversation_id=conversation_id | ||
| ): | ||
| yield |
| HttpAdapterBase, | ||
| HttpResponse, | ||
| ) | ||
| from microsoft_agents.hosting.core import ChannelServiceClientFactoryBase | ||
| from microsoft_agents.hosting.core.telemetry import spans | ||
|
|
||
| from .agent_http_adapter import AgentHttpAdapter |
| attributes = {} | ||
| attributes["activity.type"] = turn_context.activity.type | ||
| attributes["agent.is_agentic"] = turn_context.activity.is_agentic_request() | ||
| if turn_context.activity.from_property: | ||
| attributes["from.id"] = turn_context.activity.from_property.id | ||
| if turn_context.activity.recipient: | ||
| attributes["recipient.id"] = turn_context.activity.recipient.id | ||
| if turn_context.activity.conversation: | ||
| attributes["conversation.id"] = turn_context.activity.conversation.id | ||
| attributes["channel_id"] = turn_context.activity.channel_id | ||
| attributes["message.text.length"] = ( | ||
| len(turn_context.activity.text) if turn_context.activity.text else 0 | ||
| ) |
| from ._common import ( | ||
| create_agent_path, | ||
| SDKVersion, | ||
| ) | ||
| from ._common.utils import create_agent_path | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 91 out of 223 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (3)
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/transport/transcript/exchange.py:125
Exchange.from_requestassumesresponse_or_exceptionhas.text()whenstatus >= 300, but its type annotation allowsExceptionas well. Also this branch doesn't populatestatus_code, which makes error Exchanges harder to diagnose. Guard this branch withisinstance(response_or_exception, aiohttp.ClientResponse)(or similar), and includestatus_code=statusin the returned Exchange.
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py:15from email.mime import baseis unused and appears accidental. Please remove it to avoid lint/type-check noise.
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/transport/aiohttp_sender.py:75- Raising
ClientErroronresponse.status >= 300prevents anExchangefrom being created/recorded for non-2xx responses (and makes the newExchange.from_request(..., status=...)branch unreachable). Instead, letExchange.from_requesthandle non-success status codes (or create an Exchange explicitly) so failures are captured in the transcript rather than being re-raised and losing diagnostics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with spans.start_span_turn_context_send_activity(self): | ||
|
|
||
| result = await self.send_activities([activity_or_text]) | ||
| return result[0] if result else None |
There was a problem hiding this comment.
This with spans.start_span_turn_context_send_activity(...) call will fail at runtime because microsoft_agents.hosting.core.telemetry.spans currently doesn't define start_span_turn_context_send_activity (there is both a telemetry/spans.py module that is fully commented out and a telemetry/spans/ package with an empty __init__.py). Define/export the start_span_turn_context_* helpers (or update these call sites to use the new SpanWrapper classes) and remove/rename the conflicting spans.py vs spans/ package to avoid ambiguous imports.
| from opentelemetry.sdk.metrics import MeterProvider | ||
| from opentelemetry.sdk.metrics.export import InMemoryMetricReader | ||
|
|
||
| from microsoft_agents.hosting.core.telemetry import constants |
There was a problem hiding this comment.
from microsoft_agents.hosting.core.telemetry import constants will fail because telemetry/ has no constants.py at the package root (constants live under telemetry.core.constants). Either change this import to from microsoft_agents.hosting.core.telemetry.core import constants or re-export constants from microsoft_agents.hosting.core.telemetry.__init__ so callers can import it directly.
| from microsoft_agents.hosting.core.telemetry import ( | ||
| agents_telemetry, | ||
| core as common_constants, | ||
| _format_scopes, | ||
| ) | ||
|
|
||
| from . import constants, _metrics | ||
|
|
||
|
|
||
| @contextmanager | ||
| def start_span_auth_get_access_token( | ||
| scopes: list[str], auth_type: str | ||
| ) -> Iterator[None]: | ||
| with agents_telemetry.start_as_current_span( | ||
| constants.SPAN_AUTH_GET_ACCESS_TOKEN | ||
| ) as span: | ||
| span.set_attributes( | ||
| { | ||
| common_constants.ATTR_AUTH_SCOPES: _format_scopes(scopes), | ||
| common_constants.ATTR_AUTH_TYPE: auth_type, | ||
| } | ||
| ) | ||
| yield | ||
|
|
||
|
|
||
| @contextmanager | ||
| def start_span_auth_acquire_token_on_behalf_of(scopes: list[str]) -> Iterator[None]: | ||
| with agents_telemetry.start_as_current_span( | ||
| constants.SPAN_AUTH_ACQUIRE_TOKEN_ON_BEHALF_OF | ||
| ) as span: | ||
| span.set_attribute(common_constants.ATTR_AUTH_SCOPES, _format_scopes(scopes)) | ||
| yield | ||
|
|
||
|
|
||
| @contextmanager | ||
| def start_span_auth_get_agentic_instance_token( | ||
| agentic_instance_id: str, | ||
| ) -> Iterator[None]: | ||
| with agents_telemetry.start_as_current_span( | ||
| constants.SPAN_AUTH_GET_AGENTIC_INSTANCE_TOKEN | ||
| ) as span: | ||
| span.set_attribute( | ||
| common_constants.ATTR_AGENTIC_INSTANCE_ID, agentic_instance_id | ||
| ) | ||
| yield | ||
|
|
||
|
|
||
| @contextmanager | ||
| def start_span_auth_get_agentic_user_token( | ||
| agentic_instance_id: str, agentic_user_id: str, scopes: list[str] | ||
| ) -> Iterator[None]: | ||
| with agents_telemetry.start_as_current_span( | ||
| constants.SPAN_AUTH_GET_AGENTIC_USER_TOKEN | ||
| ): | ||
| span.set_attributes( | ||
| { | ||
| common_constants.ATTR_AGENTIC_INSTANCE_ID: agentic_instance_id, | ||
| common_constants.ATTR_AGENTIC_USER_ID: agentic_user_id, | ||
| common_constants.ATTR_AUTH_SCOPES: scopes, | ||
| } | ||
| ) |
There was a problem hiding this comment.
There are multiple runtime issues here: (1) core as common_constants is the telemetry.core package, so attributes like ATTR_AUTH_SCOPES/ATTR_AUTH_TYPE won't exist there (they live under telemetry.core.constants). (2) _format_scopes is not exported by microsoft_agents.hosting.core.telemetry (the function is format_scopes). (3) start_span_auth_get_agentic_user_token uses span.set_attributes(...) but the with statement does not bind as span, causing a NameError. Fix the imports to reference the correct constants/util and bind the span with as span.
| from abc import ABC, abstractmethod | ||
| from contextlib import ExitStack | ||
| from typing import ContextManager | ||
| from venv import logger | ||
|
|
||
| from opentelemetry.trace import Span | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
from venv import logger is unused and also shadows the logger = logging.getLogger(__name__) defined below, which is confusing and can lead to incorrect logger usage. Remove the venv import and keep the module-level logging.getLogger(__name__) logger only.
| if key == "": | ||
| raise ValueError("MemoryStorage.read(): key cannot be empty") | ||
| if key in self._memory: | ||
| if not target_cls: | ||
| result[key] = self._memory[key] | ||
| else: | ||
| try: |
There was a problem hiding this comment.
Inside MemoryStorage.read, target_cls is validated as non-null at the top of the method, so the inner if not target_cls: branch is dead code. Removing that branch will simplify the control flow and avoid misleading future readers about target_cls possibly being None here.
| from ..core import constants, AttributeMap | ||
| from opentelemetry.trace import Span | ||
| from microsoft_agents.activity import Activity | ||
|
|
||
| from ..core import ( | ||
| constants, | ||
| AttributeMap, | ||
| SimpleSpanWrapper, | ||
| ) |
There was a problem hiding this comment.
This file has duplicate imports for constants, AttributeMap, etc. (from ..core import constants, AttributeMap and then again importing them a few lines later). Please remove the redundant import block to avoid confusion and keep the module header clean.
| with spans.start_span_adapter_create_connector_client( | ||
| service_url=service_url, | ||
| scopes=scopes, | ||
| is_agentic_request=is_agentic_request, | ||
| ): | ||
|
|
There was a problem hiding this comment.
This new spans.start_span_adapter_create_connector_client(...) usage will fail unless microsoft_agents.hosting.core.telemetry.spans exports that helper. Given the current telemetry/spans.py vs telemetry/spans/ name conflict, spans resolves to a module/package with no start_span_* functions. Please implement/export this span helper (or switch to the new SpanWrapper classes) and remove the conflicting duplicate module/package name.
| with spans.start_span_adapter_process(activity): | ||
|
|
||
| try: | ||
| # Process the inbound activity with the agent | ||
| invoke_response = await self.process_activity( | ||
| claims_identity, activity, agent.on_turn | ||
| # Get claims identity (default to anonymous if not set by middleware) | ||
| claims_identity: ClaimsIdentity = ( | ||
| request.get_claims_identity() or ClaimsIdentity({}, False) | ||
| ) |
There was a problem hiding this comment.
Wrapping request handling in with spans.start_span_adapter_process(activity): will fail unless microsoft_agents.hosting.core.telemetry.spans actually exports start_span_adapter_process. Currently telemetry/spans.py is commented out and telemetry/spans/ has an empty __init__.py, so the spans import provides no helpers. Please implement/export this span helper and resolve the spans.py vs spans/ naming conflict.
| from microsoft_agents.hosting.core import ChannelServiceClientFactoryBase | ||
| from microsoft_agents.hosting.core.telemetry import spans | ||
|
|
||
| from .agent_http_adapter import AgentHttpAdapter |
There was a problem hiding this comment.
from microsoft_agents.hosting.core.telemetry import spans is unused in this file. Remove it (or use it) to avoid linter failures.
| with spans.start_span_storage_read(len(keys)): | ||
| await self.initialize() | ||
|
|
||
| items: list[tuple[Union[str, None], Union[StoreItemT, None]]] = await gather( | ||
| *[self._read_item(key, target_cls=target_cls, **kwargs) for key in keys] | ||
| ) | ||
| return {key: value for key, value in items if key is not None} | ||
| items: list[tuple[Union[str, None], Union[StoreItemT, None]]] = ( | ||
| await gather( | ||
| *[ | ||
| self._read_item(key, target_cls=target_cls, **kwargs) | ||
| for key in keys | ||
| ] | ||
| ) | ||
| ) | ||
| return {key: value for key, value in items if key is not None} |
There was a problem hiding this comment.
This with spans.start_span_storage_read(...) call will raise AttributeError unless the start_span_storage_* helpers are actually implemented/exported from microsoft_agents.hosting.core.telemetry.spans. Right now the repo contains both telemetry/spans.py (commented out) and telemetry/spans/ (empty __init__.py), so from ...telemetry import spans is ambiguous and provides no start_span_storage_read. Consolidate spans into a single module/package and export the start_span_storage_* context managers used here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 234 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/fluent/model_template.py:15
- There is an unused import (
from email.mime import base) that isn't referenced anywhere in this module. Please remove it to avoid lint failures and keep imports minimal.
dev/testing/microsoft-agents-testing/microsoft_agents/testing/core/transport/aiohttp_sender.py:75 - The sender raises
ClientErrorfor any HTTP status >= 300, and then immediately re-raises if a response object exists. This preventsExchange.from_request(..., status=...)from ever capturing the error body/status into the transcript, even thoughExchange.from_requesthas logic for non-2xx statuses. Consider removing the explicit raise and lettingExchange.from_requesthandle non-success statuses so tests/transcripts can record the server error details.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from microsoft_agents.hosting.core.telemetry import ( | ||
| agents_telemetry, | ||
| core as common_constants, | ||
| _format_scopes, | ||
| ) |
There was a problem hiding this comment.
This module references telemetry symbols that don't exist: core as common_constants doesn't provide ATTR_AUTH_* keys, and _format_scopes is not exported by microsoft_agents.hosting.core.telemetry (the function is named format_scopes). Please import the correct constants/functions from microsoft_agents.hosting.core.telemetry.attributes and microsoft_agents.hosting.core.telemetry.utils (or re-export them) so these spans can set attributes without raising at runtime.
| from microsoft_agents.hosting.core.telemetry import ( | ||
| resource as common_constants, | ||
| SimpleSpanWrapper, | ||
| ) | ||
| from . import metrics, constants | ||
|
|
||
| class _StorageSpanWrapper(SimpleSpanWrapper): | ||
| """Base SpanWrapper for spans related to storage operations. This is meant to be a base class for spans related to storage operations, such as retrieving or saving state, and can be used to share common functionality and attributes related to storage operations.""" | ||
|
|
||
| def __init__(self, span_name: str, *, key_count: int): | ||
| """Initializes the _StorageSpanWrapper span.""" | ||
| super().__init__(span_name) | ||
| self._key_count = key_count | ||
|
|
||
| def _callback(self, span: Span, duration: float, error: Exception | None) -> None: | ||
| """Callback function that is called when the span is ended. This is used to record metrics for the storage operation based on the outcome of the span.""" | ||
| metrics.storage_operation_duration.record(duration) | ||
| metrics.storage_operation_total.add(1) | ||
|
|
||
| def _get_attributes(self) -> dict[str, str | int]: | ||
| """Returns a dictionary of attributes to set on the span when it is started. This includes attributes related to the storage operation being performed. | ||
|
|
||
| NOTE: a dict is the annotated return type to allow child classes to add additional attributes. | ||
| """ | ||
| return { | ||
| common_constants.ATTR_KEY_COUNT: self._key_count, | ||
| } |
There was a problem hiding this comment.
common_constants is imported from microsoft_agents.hosting.core.telemetry.resource, but that module only defines SERVICE_* / RESOURCE and does not define ATTR_KEY_COUNT. This will raise AttributeError when starting the span. Use the telemetry attributes constant for key count (e.g., microsoft_agents.hosting.core.telemetry.attributes.KEY_COUNT) or define/export the expected key-count attribute constant.
| from opentelemetry.sdk.metrics.export import InMemoryMetricReader | ||
|
|
||
| from microsoft_agents.hosting.core.telemetry import constants | ||
|
|
||
| from tests.scenarios import load_scenario |
There was a problem hiding this comment.
microsoft_agents.hosting.core.telemetry does not contain a constants module, and the referenced names (SPAN_APP_ON_TURN, SPAN_ADAPTER_PROCESS, SPAN_STORAGE_READ) are not defined anywhere in the current telemetry package layout. Either add a telemetry/constants.py (and export it) that defines these names, or update the test to import the actual constants from their modules (e.g., app/telemetry/constants.py, telemetry/adapter/constants.py, storage/telemetry/constants.py) and assert on the correct span names.
| try: | ||
| run_app(APP, host="localhost", port=environ.get("PORT", 3978)) | ||
| except Exception as error: |
There was a problem hiding this comment.
aiohttp.web.run_app expects an integer port, but environ.get("PORT", 3978) may return a string when PORT is set. Convert the env var to int (with a safe fallback) before passing it to run_app.
| from typing import ContextManager | ||
| from venv import logger | ||
|
|
||
| from opentelemetry.trace import Span | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
There was a problem hiding this comment.
from venv import logger is an invalid/unused import here and it also shadows the module-level logger you define a few lines later. Please remove this import (and rely on logging.getLogger(__name__)).
| from microsoft_agents.hosting.core.telemetry import spans | ||
|
|
||
| from ._type_aliases import JSON | ||
| from .storage import Storage | ||
| from .store_item import StoreItem | ||
| from .telemetry import spans | ||
|
|
There was a problem hiding this comment.
This file imports spans twice and one of them is from microsoft_agents.hosting.core.telemetry, which currently doesn't export a spans module. This will either fail at import time or mask the intended storage telemetry spans. Keep only the import from .telemetry (or otherwise import the correct spans module) and remove the duplicate.
| endpoint = os.getenv("OTEL_EXPORTER_OTLP_ENDPOINT", "http://localhost:4317/") | ||
|
|
||
| # Configure Tracing | ||
| tracer_provider = TracerProvider(resource=resource) | ||
| tracer_provider.add_span_processor( | ||
| SimpleSpanProcessor(OTLPSpanExporter(endpoint=endpoint)) | ||
| ) |
There was a problem hiding this comment.
OTLP*Exporter classes under opentelemetry.exporter.otlp.proto.grpc expect a gRPC endpoint (typically host:port) and may not accept an http://.../ URL with a trailing slash. Consider normalizing OTEL_EXPORTER_OTLP_ENDPOINT for gRPC exporters (e.g., default to localhost:4317), and wiring OTEL_EXPORTER_OTLP_INSECURE/TLS settings so the sample works with common local collectors.
| @@ -96,38 +97,40 @@ async def process_request( | |||
|
|
|||
| activity: Activity = Activity.model_validate(body) | |||
|
|
|||
| # Get claims identity (default to anonymous if not set by middleware) | |||
| claims_identity: ClaimsIdentity = ( | |||
| request.get_claims_identity() or ClaimsIdentity({}, False) | |||
| ) | |||
|
|
|||
| # Validate required activity fields | |||
| if ( | |||
| not activity.type | |||
| or not activity.conversation | |||
| or not activity.conversation.id | |||
| ): | |||
| return HttpResponseFactory.bad_request( | |||
| "Activity must have type and conversation.id" | |||
| ) | |||
| with spans.start_span_adapter_process(activity): | |||
|
|
|||
There was a problem hiding this comment.
microsoft_agents.hosting.core.telemetry does not define/export a spans module, so this import will fail at runtime. Also, spans.start_span_adapter_process(...) is not defined in the telemetry package as currently structured. Import the correct module (e.g., microsoft_agents.hosting.core.telemetry.adapter.spans) and use the appropriate span wrapper (e.g., with spans.AdapterProcess(activity): ...), or add and export a start_span_adapter_process helper from the telemetry package.
No description provided.