Skip to content

Add model observation inference attributes - #289

Open
Pujarini Mohapatra (biswapm) wants to merge 6 commits into
microsoft:mainfrom
biswapm:users/pmohapatra/model-observation-schema
Open

Pujarini Mohapatra (biswapm) wants to merge 6 commits into
microsoft:mainfrom
biswapm:users/pmohapatra/model-observation-schema

Conversation

@biswapm

Copy link
Copy Markdown
Collaborator

Summary

Adds the privacy-safe OpenTelemetry attributes required by the Model Observation pipeline.

  • Capture gen_ai.response.model when the provider returns the canonical served model.
  • Capture gen_ai.usage.cache_read.input_tokens.
  • Capture gen_ai.usage.cache_write.input_tokens.
  • Normalize the inference gen_ai.operation.name value to lowercase OTel values, including chat.
  • Capture model endpoint information as server.address and an optional numeric server.port.
  • Support attributes supplied when the inference scope starts or recorded later when the response becomes available.
  • Add focused tests covering the emitted attribute names and value types.

Validation

The repository’s targeted inference tests, build, formatting, and linting checks pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Performance comparison

Threshold: regressions >15.0% on gating scenarios fail the build. Higher ops/s is better; positive Δ means the PR is slower.

Scenario Gating Baseline (ops/s) Candidate (ops/s) Δ % Status
azure_monitor_log yes 36,584.5 36,865.0 -0.76% ✅
azure_monitor_span yes 275,406.2 270,892.6 +1.67% ✅
otel_log no 43,293.8 42,235.1 +2.51% ✅
otel_span no 81,096.4 81,486.3 -0.48% ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Non-chat operation names are normalized to invalid values without underscores.

1 open finding
What changed in this PR

Adds model-observation attributes to A365 inference spans.

Changes:

  • Records response-model and cache-token attributes at startup or later.
  • Emits numeric endpoint ports.
  • Normalizes inference operation names and adds focused tests.
File Description
tests/​a365/​test_inference_scope.py Tests new inference attributes.
src/​microsoft/​opentelemetry/​a365/​core/​inference_scope.py Emits and updates model-observation attributes.
src/​microsoft/​opentelemetry/​a365/​core/​inference_call_details.py Adds response and cache-token fields.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/microsoft/opentelemetry/a365/core/inference_scope.py Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 13, 2026 09:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Multiword inference operations are normalized without required underscores, producing invalid OTel values.

1 open finding

🧠 Review effort: Balanced

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 13, 2026 09:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Span names still use legacy PascalCase operations instead of the newly canonicalized values.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Comment thread src/microsoft/opentelemetry/a365/core/inference_scope.py Outdated
Build activity_name from the same normalized operation name used for
gen_ai.operation.name so span names use the canonical lowercase OTel
value (e.g. 'text_completion gpt-4o') instead of the legacy enum value.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 15, 2026 09:26
@biswapm

Copy link
Copy Markdown
Collaborator Author

Fixed in bc8a5eb: \�ctivity_name\ now builds from the same normalized operation name used for \gen_ai.operation.name, so span names use canonical values (e.g. \ ext_completion gpt-4o). Added a span-name assertion to the canonical-operation-names test.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Normalized non-chat span names can cause incorrect agent-invocation baggage enrichment.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Comment thread src/microsoft/opentelemetry/a365/core/inference_scope.py
Copilot AI balanced review requested due to automatic review settings October 15, 2026 09:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Normalized non-chat operation names can cause processor misclassification and incorrect endpoint baggage propagation.

1 open finding

🧠 Review effort: Balanced

GEN_AI_PROCESSOR_OPERATION_NAMES previously only contained legacy
PascalCase InferenceOperationType.value spellings (e.g. 'TextCompletion'),
so span names using the canonical lowercase form (e.g. 'text_completion
gpt-4o', emitted by InferenceScope since the earlier activity-name fix)
were not recognized by _operation_name_from_span_name. This caused such
spans to fall through to inherited gen_ai.operation.name baggage (e.g.
'invoke_agent'), incorrectly copying invoke_agent-only attributes
(caller agent identity, server.address/port) onto inference spans.

Add the canonical lowercase spellings to the frozenset alongside the
legacy ones, and add a regression test exercising inherited invoke_agent
baggage with a canonical inference span name.
Copilot AI balanced review requested due to automatic review settings October 15, 2026 09:51
@biswapm

Copy link
Copy Markdown
Collaborator Author

Fixed in bba0434: GEN_AI_PROCESSOR_OPERATION_NAMES now also includes the canonical lowercase operation-name spellings (chat, ext_completion, generate_content) alongside the legacy PascalCase values, so span-name-based inference (_operation_name_from_span_name) recognizes canonical inference span names like ext_completion gpt-4o instead of falling through to inherited gen_ai.operation.name baggage (e.g. invoke_agent). This prevents invoke_agent-only attributes (caller agent identity, server.address/server.port) from leaking onto inference spans.

Added a regression test ( est_canonical_inference_span_name_does_not_inherit_invoke_agent_baggage) covering chat/ ext_completion/generate_content span names with inherited invoke_agent baggage, verifying common identity attributes are still enriched while invoke_agent-only attributes are withheld. Full suite: 604 passed, 25 skipped.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused changes include regression coverage and have no identified blocking issues; tests were not executed during this review.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed canonical operation normalization, span-name consistency, response/cache attributes, numeric endpoint ports, and regression coverage. No blocking findings for supported inputs.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants