You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The reason will be displayed to describe this comment to others. Learn more.
Keeping shared attribute definitions in core and framework-specific extraction in the adapters is a sound, focused approach that fits the existing architecture.
Four minor comments inline.
Outside the changed lines:
Minor: Integration guide still says InferenceScope emits Chat/TextCompletion/GenerateContent (docs/integrating-with-existing-opentelemetry.md:96). After this change InferenceScope always emits gen_ai.operation.name = chat and names spans chat <model>. This guide still says: - Manual instrumentation uses the capitalized InferenceOperationType.value, and manual and auto-instrumentation are 'inconsistent today' (line 96). - Users should filter their backend on Chat / TextCompletion / GenerateContent, and the casing difference is a tracked SDK issue (line 99). - Users should look for Chat (or chat) when verifying the integration (line 113). Anyone who builds dashboards or filters from this page will match on values the S...
The reason will be displayed to describe this comment to others. Learn more.
The separation between shared telemetry definitions and framework-specific extraction is clear and consistent with the existing architecture. The changes are narrowly scoped and straightforward to follow.
One minor comment inline.
Since my last review:
Still open: Read cache details from the generated message's UsageMetadata (libraries/microsoft-agents-a365-observability-extensions-langchain/microsoft_agents_a365/observability/extensions/langchain/utils.py). libraries/microsoft-agents-a365-observability-extensions-langchain/microsoft_agents_a365/observability/extensions/langchain/utils.py:329-330,395-418 still obtains usage exclusively from llm_output.tok
Still open: Whitespace model is emitted as the request model and span name (libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/inference_scope.py). libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/inference_call_details.py:28-30 accepts model=' ' when responseModel='gpt-4.1'. In the same package, inferen
Still open: Integration guide still says InferenceScope emits Chat/TextCompletion/GenerateContent (docs/integrating-with-existing-opentelemetry.md). docs/integrating-with-existing-opentelemetry.md:96,99,113 still describes capitalized manual inference operations and recommends matching them. libraries/microsoft-agents-a365-observability-core/micro
Still open: New agent_type is a plain string, but design.md documents it as the AgentType enum (libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/agent_details.py). libraries/microsoft-agents-a365-observability-core/docs/design.md:289 still documents agent_type as AgentType | None. The implementation declares Optional[str] in agent_details.py:33, while models/age
…g, cache tokens
Addresses 6 unresolved review threads on PR #282:
1. AgentDetails.agent_type: moved to the end of the dataclass so existing
positional callers aren't shifted; type widened to Optional[Union[AgentType, str]].
2. InferenceCallDetails: moved responseModel/cacheReadInputTokens/
cacheWriteInputTokens to the end of the dataclass to preserve positional
constructor compatibility.
3. LangChain utils.token_counts(): now reads normalized cache token details
from the generated AIMessage's usage_metadata (both direct attribute and
dumpd kwargs-serialized forms) before falling back to llm_output-based
token_usage, and also handles Anthropic's flat raw
cache_read_input_tokens/cache_creation_input_tokens fields. Added tests
using the real message shape instead of a synthetic llm_output shape.
4. InferenceCallDetails.__post_init__: normalizes model/responseModel by
stripping whitespace and treating blank values as None, so a
whitespace-only model no longer produces a blank gen_ai.request.model /
malformed span name when responseModel is set.
5. AgentType enum handling: OpenTelemetryScope and BaggageBuilder.agent_type
now accept AgentType | str | None and emit .value for enum members so
OTel doesn't silently drop AgentType.FOUNDRY-style values.
6. InferenceScope operation naming: added InferenceOperationType.canonical_name
(maps CHAT/TEXT_COMPLETION/GENERATE_CONTENT to their canonical lowercase
semconv spelling) and use it for both the span name prefix and
gen_ai.operation.name, replacing the hardcoded "chat" value. Updated the
Agent365 exporter's GEN_AI_OPERATION_NAMES allowlist to include the
canonical lowercase names (kept legacy PascalCase entries), fixed the
PR's own test that wrongly asserted "chat" for a TEXT_COMPLETION input,
and updated docs/design.md accordingly.
Also added CHANGELOG.md entries for both affected packages.
All changes verified with the full core + langchain test suites
(278 passed, plus the 5 newly added tests) via `pytest tests/observability/core/
tests/observability/extensions/langchain/ -m "not integration"`.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
agent_details.py (dataclass field order) — Moved the new agent_type field to the end of AgentDetails, after all pre-existing fields, so existing positional callers (e.g. 8th positional arg for tenant_id) are no longer shifted.
inference_call_details.py (dataclass field order) — Moved responseModel, cacheReadInputTokens, and cacheWriteInputTokens to the end of InferenceCallDetails, restoring the pre-PR positional field order so a call like InferenceCallDetails(op, model, provider, 100, 50, ...) binds correctly again.
langchain/utils.py (cache token extraction, minor) — token_counts() now reads normalized usage_metadata.input_token_details/output_token_details from the generated AIMessage (handling both a live BaseMessage object and the dumpd message.kwargs.usage_metadata serialized form) before falling back to provider-specific llm_output lookups, and also handles Anthropic's flat raw cache_read_input_tokens/cache_creation_input_tokens. Added test_cache_token_counts_read_from_generated_message_usage_metadata and test_cache_token_counts_fall_back_to_raw_anthropic_flat_fields, which use the real message shape (not a synthetic llm_output shape).
inference_scope.py (whitespace model, minor) — InferenceCallDetails.__post_init__ now strips model/responseModel and normalizes blank/whitespace-only values to None before validation, so a whitespace-only model with a valid responseModel no longer leaks into the span name or gen_ai.request.model. Added test_whitespace_only_model_is_normalized_when_response_model_present.
agent_details.py / opentelemetry_scope.py / baggage_builder.py (AgentType enum, minor) — Widened the agent_type type hint to Optional[Union[AgentType, str]] and updated OpenTelemetryScope and BaggageBuilder.agent_type to emit .value when given an AgentType enum member, so AgentType.FOUNDRY no longer gets silently dropped by OTel. Added test_agent_type_enum_member_emits_string_value.
inference_scope.py (operation name hardcoding, significant) — Added InferenceOperationType.canonical_name (maps CHAT/TEXT_COMPLETION/GENERATE_CONTENT to their canonical lowercase semconv spelling) and replaced the hardcoded CHAT_OPERATION_NAME usage in both the span-name prefix and gen_ai.operation.name with details.operationName.canonical_name. Updated the Agent365 exporter's GEN_AI_OPERATION_NAMES allowlist in exporters/utils.py to include the lowercase canonical names (chat, text_completion, generate_content) in addition to the legacy PascalCase aliases, since spans can now genuinely emit those values. Fixed the PR's own test that incorrectly asserted "chat" for a TEXT_COMPLETION input (now asserts "text_completion"), added test_inference_scope_operation_name_mapping_per_type covering all three enum values, and updated docs/design.md to describe the per-operation canonical mapping instead of a chat-only description.
Also added CHANGELOG.md entries under [Unreleased] in both microsoft-agents-a365-observability-core and microsoft-agents-a365-observability-extensions-langchain.
…ain utils
- Remove erroneous mapping of output_token_details.reasoning (a token
count) to GEN_AI_RESPONSE_FINISH_REASONS_KEY in token_counts(); that
attribute is reserved for actual finish reasons.
- Rewrite _message_usage_metadata() to aggregate usage across ALL batch
entries in outputs["generations"] instead of only the first, so
batched LLM calls no longer silently drop usage for later inputs.
Only the first completion with usage data per batch entry is used to
avoid double-counting alternative completions (n > 1).
- Add _merge_usage_metadata() helper to recursively sum numeric usage
fields (including nested detail dicts) across batch entries.
- Add regression tests for both fixes.
- Reformat utils.py and test_wrapper_langchain.py with ruff to fix CI
format-check failure (ruff format --check .).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixed CI failure and the 2 new review comments (commit f722e88):
CI fix:
Python SDK (3.11) job was failing the format tox env (ruff format --check .) on utils.py and test_wrapper_langchain.py. Reformatted both files; verified locally against the CI-pinned ruff==0.15.0.
Review comment fixes:
Reasoning tokens mislabeled as finish reasons (utils.py line 407): removed the erroneous output_token_details.reasoning → GEN_AI_RESPONSE_FINISH_REASONS_KEY mapping that I introduced in the previous round — that's a token count, not a finish reason, and there's no dedicated reasoning-tokens attribute defined yet.
Batched usage only read the first input (utils.py line 413, _message_usage_metadata): rewrote it to aggregate usage across all batch entries in outputs["generations"] instead of only generations[0], so multi-input batched LLM calls no longer silently drop usage for later inputs. Added a _merge_usage_metadata() helper that sums numeric fields (incl. nested input_token_details/output_token_details) across batch entries, while still only using the first completion-with-usage per batch entry to avoid double-counting alternative completions when n > 1.
All 280 tests pass locally (tests/observability/core + tests/observability/extensions/langchain), and ruff format --check / ruff check --preview both pass on the touched files with the CI-pinned ruff version.
The documented cache attribute names use underscores where the emitted constants use dots. Queries using these names will miss the exported counts. Document gen_ai.usage.cache_read.input_tokens and gen_ai.usage.cache_write.input_tokens instead.
Replace Any usage values with concrete token types
The new _merge_usage_metadata and _message_usage_metadata helpers use Any for usage values and the accumulator, leaving recursive token addition unchecked. Use concrete usage/token-detail types for the normalized result and accumulator, and narrow serialized inputs from object at the extraction boundary. Update both helpers together so downstream token values remain type checked.
The reason will be displayed to describe this comment to others. Learn more.
Keeping common telemetry definitions in core and framework-specific extraction in the adapters is a good fit for this SDK. The changes are focused and generally straightforward to follow.
Three minor comments inline.
Since my last review:
Still open: Integration guide still says InferenceScope emits Chat/TextCompletion/GenerateContent (docs/integrating-with-existing-opentelemetry.md). docs/integrating-with-existing-opentelemetry.md:96,99,113 still describes PascalCase manual operation names and recommends matching them. libraries/microsoft-agents-a365-observability-core/microsoft_a
Address review feedback on canonical operation and span-name changes, model identity validation, request/response model separation, and normalized cache-inclusive input-token totals. Update the existing OpenTelemetry integration guide without changing runtime behavior.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The new _merge_usage_metadata() and _message_usage_metadata() helpers introduce Any in their parameters, return type, and accumulator, contrary to the repository's typing requirement. This leaves recursive merging and accumulator addition unchecked. Use LangChain's UsageMetadata for normalized usage, and concrete types or object with narrowing for serialized data; narrow accumulator values before adding them.
The reason will be displayed to describe this comment to others. Learn more.
The separation between shared telemetry definitions and framework-specific extraction fits the SDK well. The changes are focused, and the additive APIs follow the existing tracing abstractions.
One minor comment inline.
Since my last review:
Fixed: Integration guide still says InferenceScope emits Chat/TextCompletion/GenerateContent (docs/integrating-with-existing-opentelemetry.md)
Fixed: Operation-name casing change is filed under Fixed and misstates the previous behavior (libraries/microsoft-agents-a365-observability-core/CHANGELOG.md)
Fixed: InferenceCallDetails now raises on a missing or blank model, and the CHANGELOG doesn't say so (libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/inference_call_details.py)
Fixed: LangChain CHANGELOG omits the request/response model split and the input-token change (libraries/microsoft-agents-a365-observability-extensions-langchain/CHANGELOG.md)
The reason will be displayed to describe this comment to others. Learn more.
Approving. These are non-blocking, but please address them before merge:
Should-fix
langchain/utils.py ~L365-369: the OpenAI completion_tokens_details.reasoning_tokens value is still mapped to gen_ai.response.finish_reasons. f722e88 removed the same mapping from the usage_metadata loop but missed this one, so spans still get an integer token count in finish_reasons. Suggest dropping this entry. No test currently covers it.
openai/utils.pyget_attributes_from_response: obj.model now goes to gen_ai.response.model instead of gen_ai.request.model. Nothing else sets the request model on ResponseSpanData spans, so those spans lose gen_ai.request.model entirely. Please add an OpenAI extension CHANGELOG entry for this breaking change and for the new cache_read attribute, or keep emitting the request model as well.
langchain/CHANGELOG.md: "Batched generations retain aggregate llm_output usage rather than replacing it with a single message's usage" doesn't match the code, which sums message.usage_metadata across batch entries and prefers that over llm_output.
Consider
InferenceCallDetails.__post_init__ raising ValueError means telemetry setup can now break a user's request path. Logging a warning and falling back may be safer for an observability SDK.
text_completion and generate_content spans are now exported to Agent 365. Please confirm the ingest service accepts them.
token_counts can yield input/cache_read tokens twice (once from llm_output, once from usage_metadata), and the last value wins. That's correct today but fragile.
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
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.
Krishnadheeraj (DheerajPannala)
left a comment
Rick Brighenti (rbrighenti)
left a comment
Summary
chatso Maven model observation ingests themValidation
44 passed, 2 subtests passedacross core inference, baggage, OpenAI, and LangChain observability tests