Skip to content

fix(hermes): preserve provider response IDs - #247

Open
sipercai wants to merge 3 commits into
mainfrom
fix/hermes-provider-response-id
Open

fix(hermes): preserve provider response IDs#247
sipercai wants to merge 3 commits into
mainfrom
fix/hermes-provider-response-id

Conversation

@sipercai

@sipercai sipercai commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR makes the Hermes instrumentation prefer the model provider's operation/request identifier for gen_ai.response.id instead of the synthetic response identifier produced by Hermes.

It adds a shared util-genai helper for provider-first response-ID extraction with framework fallback, then uses that helper in Hermes as the first consumer. Hermes observes request-local OpenAI-compatible chat.completions.create responses and streaming chunks, isolates retry attempts across worker threads, propagates the selected identifier to LLM and aggregate AGENT spans, and falls back to the Hermes response identifier when the provider does not expose one.

For providers such as DashScope, request_id intentionally has higher priority than the OpenAI-compatible completion id. A later streaming usage trailer carrying request_id can therefore replace an earlier chunk id. Transport-only metadata such as _request_id and HTTP headers is not read implicitly.

The Hermes package is also aligned with the OpenTelemetry release set used by LoongSuite 0.7.0: compatible-release floors of API/SDK 1.39.1 and instrumentation/semantic-conventions 0.60b1, with the exact set pinned in tests.

Fixes # (N/A)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • tox -e precommit
  • pyright util/opentelemetry-util-genai/src/opentelemetry/util/genai/response_id.py
  • pytest -q util/opentelemetry-util-genai/tests/test_response_id.py instrumentation-loongsuite/loongsuite-instrumentation-hermes-agent/tests/test_telemetry_spec.py
  • Built both changed wheels and installed them together in a fresh Python 3.11 environment using standard PyPI dependency resolution.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Validation Evidence

Spec and Scope

  • Approved behavior: prefer a provider-returned request_id, id, or response_id; fall back to the Hermes response ID only when no provider value is available.
  • Shared abstraction: normalize, extract, and resolve response IDs while keeping field priority caller-configured.
  • First consumer: Hermes OpenAI-compatible chat completions, including synchronous, streaming, retry, tool-loop, and worker-thread paths.
  • Non-goals: bulk migration of other instrumentations, Responses API, Anthropic Messages API, and transport-header extraction.

Local Checks

Check Result Evidence
Static readiness pass LoongSuite PR readiness checker reported no blocking findings.
Precommit pass Ruff, Ruff format, uv-lock, and rstcheck passed.
Pyright pass The changed response_id.py reports 0 errors, 0 warnings, and 0 information messages.
Focused tests pass 57 response-ID and Hermes telemetry tests passed.
Dependency resolution pass Fresh wheel installation resolved API/SDK 1.39.1 and instrumentation/threading/semantic-conventions 0.60b1; package compatibility check passed.
Independent Qoder review pass Two review rounds completed with no blocking defects; the streaming trailer priority edge identified in review was fixed and covered by a focused test.
Privacy scan pass No credentials, private trace IDs, local user paths, or generated telemetry artifacts are included in the diff.

Real E2E Matrix

Scenario Status Evidence
non-streaming pass An isolated Hermes 0.19.0 run through DashScope exported the provider identifier on both LLM and AGENT spans.
streaming pass Streaming exported a provider identifier with TTFT and no Hermes stream-* fallback.
concurrency pass Two simultaneous calls produced distinct provider identifiers without cross-call contamination.
agent/tool/ReAct pass A real tool loop produced two LLM calls around one TOOL call; AGENT retained the final provider identifier.
tool-heavy N/A Response-ID capture is provider-call scoped; the two-LLM tool loop exercises aggregation, while multiple tool definitions do not change the capture path.
error path pass A bounded connection-error run emitted ERROR LLM/STEP spans without inventing a provider response ID.

Telemetry and Weaver

Check Status Evidence
Span tree / span kinds pass The isolated run captured 45 spans locally and the backend readback returned the same 45 spans across ENTRY, AGENT, STEP, LLM, and TOOL kinds.
Content capture modes pass Response-ID selection remains independent of message-content capture; no-content behavior remains covered by focused tests.
Concurrency isolation pass Deterministic threaded tests and the real two-call run both kept response IDs invocation-local.
Weaver live-check pass Eight real-provider LLM samples were filtered to remove content and trace identifiers, then passed the LoongSuite GenAI advice profile with no violations.

CI

  • The previous remote head passed all checks except typecheck, which reported the two Pyright Unknown-type errors fixed by this update.
  • The PR remains a draft while the fresh full-matrix checks run for this commit.
  • The full local tox typecheck environment was not used as final evidence because it bootstraps large unrelated SDK packages; the changed file passed Pyright directly, and GitHub CI remains the authoritative full typecheck gate.

@sipercai
sipercai marked this pull request as ready for review July 22, 2026 08:00
@ralf0131

Copy link
Copy Markdown
Collaborator

Summary

Introduces provider response ID capture for Hermes LLM spans via a thread-local _ProviderResponseAttempt mechanism. The design is clean: provider calls are wrapped transparently, response IDs are prioritized (DashScope request_id > OpenAI id), and thread-local state is properly saved/restored. Streaming responses are handled via a transparent _ProviderStreamProxy that intercepts the final chunk without changing iteration behavior. 842 lines with strong test coverage including retry, thread isolation, error propagation, and stale-attempt edge cases.

Findings

No blocking issues. One minor note:

  • [Info] pyproject.toml — Dependency specifiers tightened from range (>= 1.37.0, < 1.40) to pin (~= 1.39.1). This narrows the lower bound but aligns with the LoongSuite release set as stated in the changelog. Intentional and acceptable.

Code Quality Highlights

  • _wrap_provider_create gracefully handles read-only resources (e.g., async iterators) with a _READONLY sentinel
  • Broad except Exception in extract_response_id is justified — telemetry must never break model calls
  • _PROVIDER_CREATE_WRAPPED guard prevents double-wrapping
  • Thread-local cleanup uses proper save/restore pattern with _MISSING_THREAD_LOCAL sentinel

Automated review by github-manager-bot

@sipercai
sipercai force-pushed the fix/hermes-provider-response-id branch from 89e6c5f to 54650a2 Compare July 28, 2026 05:51

@ralf0131 ralf0131 left a comment

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.

Summary

LGTM — This PR correctly implements provider-first response ID extraction for Hermes instrumentation. The implementation:

  1. Well-designed utility moduleresponse_id.py provides clean abstractions for response ID extraction with proper field ordering and error handling
  2. Robust thread isolation — Thread-local storage correctly isolates concurrent requests, with proper cleanup in finally blocks
  3. Correct priority handling — DashScope request_id > OpenAI-compatible id > framework response ID
  4. Graceful degradation — Telemetry never breaks the model call, even with read-only provider resources
  5. Comprehensive tests — Excellent coverage including streaming, retries, thread isolation, error handling, and fallback scenarios

The dependency version alignment (~= 1.39.1) is appropriate for LoongSuite's release strategy.


Automated review by github-manager-bot

@ralf0131 ralf0131 left a comment

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.

LGTM. Clean fix that correctly prefers provider response IDs over framework-generated ones.

Highlights:

  • response_id.py utility is well-designed: proper normalization (bool rejection, whitespace trimming), supports both Mapping and attribute access, handles lazy-load SDK exceptions gracefully
  • ProviderClientWrapper cleanly intercepts the OpenAI-compatible client to capture provider IDs without disrupting the call chain
  • Thread-safe retry isolation with per-attempt response ID tracking
  • Good test coverage including edge cases (raising properties, transport IDs, empty strings)

Automated review by github-manager-bot

@ralf0131 ralf0131 left a comment

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.

Summary

Fixes gen_ai.response.id to prefer provider-supplied identifiers (including DashScope request_id) over Hermes's synthetic response ID. Introduces a shared response_id.py helper in util-genai and a ProviderClientWrapper for invocation-local ID capture with proper thread isolation via threading.local().

Findings

  • [Info] The priority system (request_id > id > response_id) is well-documented and the streaming trailer edge case (late request_id replacing earlier chunk id) is correctly handled with the priority counter.
  • [Info] Good fail-open behavior: read-only provider resources (slots/descriptors) gracefully fall back without breaking the model call.

Suggestions

  • The comprehensive test matrix (streaming, retry, thread isolation, error paths, stale attempt prevention) provides strong confidence. The OTel dependency alignment (1.39.1/0.60b1) is a good housekeeping addition.

Automated review by github-manager-bot

@ralf0131

Copy link
Copy Markdown
Collaborator

⚠️ Merge conflict detected

This PR has conflicts with the main branch and cannot be merged. Please rebase or merge main into your branch and resolve the conflicts:

git fetch origin
git checkout fix/hermes-provider-response-id
git rebase origin/main
# resolve conflicts, then:
git push --force-with-lease

This is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved.


Automated notification by github-manager-bot

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.

4 participants