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
This is the Python port of microsoft/Agent365-nodejs#290. Agent 365 observability (OBS) export now uses only the app-only S2S route, so customers no longer need to admin-consent the delegated Agent365.Observability.OtelWrite permission for telemetry.
The exporter always uses S2S.build_export_url() always returns /observabilityService/tenants/{tenantId}/otlp/agents/{agentId}/traces?api-version=1.
Agent365ExporterOptions.use_s2s_endpoint is deprecated and ignored, even when False.
There is no fallback to /observability. HTTP 401, 403 and 404 fail the batch without a retry or a delegated route.
The exporter authenticates only with the configured app-only resolver.
An empty token now fails that identity group without sending a request; previously the request went out without an Authorization header. Resolver exceptions still fail the group.
Async resolvers (the type Agent365ExporterOptions already declared) are now awaited when the export runs outside an active event loop, such as the BatchSpanProcessor worker. Previously the coroutine object ended up in the Authorization header.
When the exporter is enabled without a resolver, the existing console-exporter fallback is kept.
Hosting cache: an app-only resolver replaces delegated exchange_token.
keeps one cache entry per (agent_id, tenant_id), keyed by the tuple so IDs that contain a colon can't collide;
keeps each refresh lock on its entry, so eviction and invalidate_all() reclaim it; eviction skips an entry whose refresh is still running, and the next insertion trims any overflow;
returns the cached token while it is still valid, with a 60 s refresh skew;
expires tokens that have no exp claim after a fallback TTL of 1 h;
retries transient errors, and clears the entry then re-raises on failure;
removes the entry on invalidate_token(), so a refresh still in flight can't repopulate it.
get_observability_token() keeps its async signature. It is now a pure cache read that returns the cached app-only token, or None, and never exchanges a token.
The delegated register_observability(...) now logs once and does nothing. It never calls Authorization.exchange_token.
Recommended exporter resolver: async def token_resolver(agent_id, tenant_id): return await cache.refresh_observability_token(agent_id, tenant_id, acquire_app_only_obs_token). The hosting README documents the constraints for async resolvers on the exporter thread.
The default OBS scope is /.default.get_observability_authentication_scope() now returns api://9b975845-388f-4429-889e-eab1ef63949c/.default, which is what client credentials requires. This matches PROD_OBSERVABILITY_SCOPE in the Node SDK.
Docs, versioning and unchanged areas.
READMEs, design docs and CHANGELOGs are updated.
versioning/TARGET-VERSION goes from 1.0.0 to 2.0.0.
Workload MCP/Graph/OBO authentication and the Spectra exporter are unchanged.
Compatibility
This is a breaking change, hence the major version bump:
Delegated/OBO OBS tokens are no longer an export path; the S2S route rejects scp tokens.
Code that used register_observability together with await get_observability_token must switch to refresh_observability_token with an app-only resolver. Until it does, those calls don't raise: they log the migration error once and return None, so telemetry stops but the app's request path keeps working.
Was /observability/... unless use_s2s_endpoint=True
test_export_config_consistency.py is updated to the new values. The live check below uses a token for this scope, sent to this host and path.
Validation
Checks:ruff check and ruff format --check pass. uv run --frozen pytest tests/ -m "not integration" gives 832 passed, 3 skipped and 9 deselected. The baseline on main is 800 passed and 3 skipped. The delegated-exchange cache tests are replaced by tests for the app-only resolver.
Mutation checks: each check temporarily reverted one behavior and confirmed that a test fails. The behaviors covered:
S2S route
empty token is not sent
/.default scope
register_observability logs once and does nothing
cache reuse and concurrent-refresh dedupe
bare TimeoutError and ConnectionResetError are retried
an in-flight refresh can't undo invalidate_token()
IDs that contain a colon don't share a cache entry
refresh locks are reclaimed with their entries, and eviction skips a refresh that is still running
console fallback when there is no resolver
non-callable resolver raises TypeError
get_observability_token stays async
Live check against the production Agent 365 endpoint, using a registered agent instance in a test tenant:
Token acquisition: blueprint FMI assertion, then the agent's client_credentials grant for the OBS /.default scope. The token has idtyp=app, no app roles and no scp, so no OtelWrite grant is involved.
One invoke_agent span was exported through the SDK exporter, in the workspace environment, in four configurations:
Configuration
Result
Default options
POST /observabilityService/tenants/{t}/otlp/agents/{a}/traces?api-version=1 → 200, SUCCESS
use_s2s_endpoint=False (deprecated)
Same S2S route → 200, SUCCESS
Resolver returns an empty token
FAILURE, no HTTP request sent, error logged
Async resolver backed by AgenticTokenCache.refresh_observability_token, two exports
Both exports → 200. The resolver was called once and received the default scope api://9b975845-388f-4429-889e-eab1ef63949c/.default. The cache was reused across the exporter's separate event loops.
The reason will be displayed to describe this comment to others. Learn more.
PR Review Agent — needs work
Risk: Medium. The S2S-only route, /.default scope, fail-closed exporter behavior, major-version bump, and migration documentation are internally consistent, but the new hosting cache has two unresolved should-fix lifecycle/isolation defects.
Should-fix
agent_token_cache.py:79 — the delimiter-built cache key aliases distinct (agent_id, tenant_id) pairs, allowing the second pair to receive the first pair's cached app-only token without invoking its resolver.
agent_token_cache.py:190 — _map is capped at 10,000 entries, but _key_locks is never reclaimed on eviction or invalidate_all(), so identity churn makes a long-lived host grow without the advertised bound.
Panel roll-up
Security: found the composite-key identity aliasing defect; app-only scope and empty-token fail-closed behavior otherwise checked clean.
Privacy: no new persistence, retention, or telemetry-content egress; tenant/agent partitioning depends on fixing the cache-key alias.
Performance: found unbounded per-identity lock retention; retry and expiry work is otherwise bounded.
Customer Service: migration, no-resolver fallback, and resolver-failure diagnostics are documented and tested.
Business / COGS: no material ingestion, storage, query, or dependency-cost regression.
Senior Engineer: cache collision and churn cases are missing from the otherwise strong new regression suite.
Architect: route/scope/version contract is coordinated; lock lifecycle does not currently honor the cache's capacity contract.
Feedback ledger: no microsoft/Agent365-python ledger existed, and the existing exact-head Copilot review reported no overlapping findings, so nothing was suppressed. All 10 exact-head GitHub checks completed successfully. Local targeted execution was blocked by a transient PyPI TLS handshake failure while restoring the workspace; both defects were independently reproduced against the exact-head cache implementation.
Approval gate: not approved because 2 should-fix findings remain open. Merge remains subject to branch protection and the coordinated cross-SDK release gate.
- Key the cache by an (agent_id, tenant_id) tuple instead of
"agent_id:tenant_id", so IDs that contain a colon cannot share another
identity's cached token. make_key (new in this PR) becomes private.
- Keep each refresh lock on its cache entry, so capacity eviction and
invalidate_all() reclaim it. Eviction skips entries with a refresh in flight.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
- When every cached entry has a refresh in flight, a new identity can take
the cache past its bound. Evict idle entries until there is room, so the
next insertion trims any overflow instead of keeping it.
- Guard the one-time removed-registration log with the cache lock.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
The reason will be displayed to describe this comment to others. Learn more.
PR Review Agent — re-review complete, approval still blocked on 97c8201
Re-reviewed 5d40056..97c8201 and the updated PR requirements. No new inline findings were added.
Accepted: the cache-key isolation fix. agent_token_cache.py:71,79-81 now uses (agent_id, tenant_id) as the dictionary key, and test_agent_token_cache.py:312-323 distinguishes the two separator-bearing pairs.
Partially accepted: moving the refresh lock onto _Entry removes the separate unbounded _key_locks registry and lets ordinary eviction / invalidate_all() reclaim synchronization state. However, on this reviewed SHA, _get_or_create_entry() evicts at most one idle entry (agent_token_cache.py:172-186); when all entries are refreshing it inserts beyond capacity, and subsequent insertions do not shrink the overflow. The built-in reviewer already filed this exact residual, so it is not duplicated here.
Newer-head response: the author's 2a7b006 follow-up changes eviction to trim idle overflow in a loop and adds the next-insertion regression. That source-backed fix is accepted for the newer head, but it is outside this queued 97c8201 review and will be evaluated by the next watcher tick.
Approval gate: held for the existing source-valid cache-capacity finding on 97c8201. All 10 substantive checks for this SHA completed successfully. Merge also remains subject to branch protection and the coordinated cross-SDK release gate.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Standard timeout and connection-reset exceptions bypass the intended transient retry behavior, and the public hosting resolver type conflicts with its documented callback signature.
This exported alias says callbacks must accept any Sequence[str], but _acquire_token always supplies a list[str] and the README documents a resolver whose third parameter is list[str]. Because callable parameters are contravariant, that documented callback is incompatible with this alias under static type checking. Use list[str] here to match the actual API (or change the implementation and documentation consistently).
Retry resolver rejects bare timeout and connection reset errors
Bare TimeoutError() and ConnectionResetError() instances commonly have empty messages, so this message-only check classifies them as non-retriable and the resolver fails on the first attempt. Check the standard transient exception types before falling back to message/status inspection so the advertised retry behavior covers normal timeout and reset failures.
…s list
- Treat TimeoutError and ConnectionError (including ConnectionResetError) as
transient before the message and status checks. Bare instances have empty
messages, so they were not retried.
- Type the ObservabilityTokenResolver scopes parameter as list[str], which is
what the cache passes and what the README documents.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Copilot's latest summary listed two "previously missed" items, both fixed in 8ae0e73:
Retry on standard transient errors: bare TimeoutError() and ConnectionResetError() have empty messages, so the message-only check didn't retry them. The cache now checks for TimeoutError and ConnectionError before looking at the message or status. The test test_refresh_retries_standard_transient_exceptions fails if that check is removed.
Resolver type:ObservabilityTokenResolver now types scopes as list[str]. That's what the cache passes and what the README documents.
Full suite: 828 passed / 3 skipped, and ruff is clean.
invalidate_token() can lose an invalidation when a refresh is in flight: the resolver can resume after _clear_token(entry) and write its token back into the same cached entry. Remove the entry from _map instead, as invalidate_all() does, so any in-flight refresh only updates a detached object and cannot repopulate the cache after invalidation.
The reason will be displayed to describe this comment to others. Learn more.
PR Review Agent - re-review complete; one approval blocker remains
Re-reviewed 97c8201..4485092, the edited PR requirements, every author response, and the live exact-head review record. No duplicate inline findings were added.
Accepted - cache capacity / lock lifecycle:2a7b006 now evicts idle entries in a loop before insertion, and test_eviction_keeps_in_flight_entry_then_trims_overflow proves the next insertion restores the configured bound while preserving an in-flight entry. The original lock-registry/capacity finding is resolved at this head.
Accepted - transient retries:8ae0e73 checks TimeoutError and ConnectionError before message/status inspection; test_refresh_retries_standard_transient_exceptions covers bare timeout and connection-reset exceptions.
Accepted - resolver type:8ae0e73 changes ObservabilityTokenResolver to list[str], matching the actual resolver argument and documented callback.
Accepted - bare scope strings:4485092 wraps a string as one scope, with test_single_scope_string_is_not_split_into_characters.
Approval blocker: the exact-head Copilot review at 18:26:54Z identified a separate invalidation race. In agent_token_cache.py:149-155, invalidate_token() clears the same _Entry object while an in-flight _acquire_token() can resume and write its result back at lines 221-229. Thus a completed invalidation can be immediately undone. Removing the keyed entry under _lock (as invalidate_all() does), plus an opposing in-flight regression, would preserve invalidation semantics. This existing finding is source-confirmed and is not duplicated inline.
Approval gate: held with 1 open should-fix finding. All 10 exact-head checks completed successfully. The local targeted run was blocked during dependency restoration by a PyPI TLS handshake failure; GitHub's Python 3.11/3.12 jobs are green. Merge remains subject to branch protection and the PR's coordinated cross-SDK merge order.
- invalidate_token() now removes the cache entry under the cache lock, as
invalidate_all() and the .NET cache do. A refresh that is still in flight
writes to the removed entry, so it can no longer repopulate the cache after
the invalidation completes.
- refresh_observability_token() returns the token it checked. A concurrent
invalidation between the usability check and the return could otherwise
make it return None.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
invalidate_token() can no longer be undone. It now removes the entry under the cache lock, like invalidate_all() does and like the .NET InvalidateToken. A refresh still in flight writes to the removed entry and returns its token to its own caller, but the cache doesn't keep it. The next refresh acquires a new token. The regression test test_invalidate_token_is_not_undone_by_in_flight_refresh reproduced the problem before the fix.
Same interaction, early-return path:refresh_observability_token() read entry.token twice. An invalidation from another thread between the usability check and the return could make it return None, although its return type is str. It now returns the token it checked. The test test_refresh_returns_the_cached_token_it_checked also failed before the fix.
Temporarily undoing either fix makes its test fail. The concurrency tests passed 10 runs in a row. Full suite: 831 passed / 3 skipped, ruff clean.
The reason will be displayed to describe this comment to others. Learn more.
PR Review Agent - re-review complete; one packaging blocker remains
Re-reviewed 4485092..2b3710c, the edited PR requirements, every author response, and the live exact-head review record. No duplicate inline findings were added.
Author-fix adjudication
Accepted - identity isolation: the tuple cache key and separator-bearing regression remain correct.
Accepted - lock/capacity lifecycle: entry-owned locks, looped idle eviction, overflow trimming on the next insertion, and the locked log-once guard remain correct.
Accepted - transient retries and resolver type: standard timeout/connection errors are classified before message inspection, and the callback alias matches the list[str] actually supplied.
Accepted - single-token invalidation:2b3710c removes the keyed entry under _lock; an in-flight resolver can update only the detached entry, so it cannot repopulate the cache. test_invalidate_token_is_not_undone_by_in_flight_refresh discriminates the old behavior.
Accepted - cached-token return race: the refresh path now returns the non-None token snapshot it checked. test_refresh_returns_the_cached_token_it_checked fails against the prior double-read.
Remaining approval blocker
The exact-head Copilot thread at agent_token_cache.py:20 is source-valid and not duplicated here. The hosting package now imports microsoft_agents_a365.runtime.environment_utils, but libraries/microsoft-agents-a365-observability-hosting/pyproject.toml does not declare microsoft-agents-a365-runtime; workspace tests mask the missing direct dependency because observability-core currently brings runtime transitively. Please declare the package's direct dependency so the published wheel metadata matches its imports.
Panel roll-up
Security / Privacy: the invalidation race is fixed; no new token-isolation, claim-validation, logging-content, or cross-tenant issue was introduced by this delta.
Performance / COGS: the delta adds no new unbounded registry, retry, storage, ingestion, or query cost.
Customer Service: the new concurrency behavior matches the updated operator/developer contract; the packaging dependency remains the only supportability risk.
Senior Engineer / Architect: both race regressions are mutation-sensitive, but the package boundary is incomplete until hosting declares runtime directly.
Approval gate: held with 1 open should-fix finding. All exact-head GitHub checks visible at review time completed successfully. Local targeted execution was blocked during dependency restoration by the same PyPI TLS handshake failure seen previously; source inspection and the exact-head Python 3.11/3.12 jobs support accepting both race fixes. Merge also remains subject to branch protection and the PR's coordinated cross-SDK order.
…endency
The hosting token cache now imports get_observability_authentication_scope
from microsoft-agents-a365-runtime. It was only available transitively through
observability-core; declare it directly so the published metadata matches the
imports. The build pins it to the same version like other internal packages.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
libraries/microsoft-agents-a365-observability-hosting/pyproject.toml now declares microsoft-agents-a365-runtime directly. It's listed by name only, per verify_constraints.py, and it's already in the root [tool.uv.sources]. uv.lock has the matching edge.
I built the hosting wheel the way CI does (PYTHONPATH=versioning/helper, version 2.0.0). Its METADATA now contains Requires-Dist: microsoft-agents-a365-runtime==2.0.0, pinned exactly like observability-core, so the published metadata matches the import.
verify_constraints.py passes, as do the dependency-constraint tests, ruff and the full suite (831 passed / 3 skipped). The Copilot thread at agent_token_cache.py:20 is answered and resolved.
The configured exporter is wrapped by _EnrichingBatchSpanProcessor, whose batch worker performs exports triggered by force_flush() and shutdown(). Calling those APIs from an event-loop thread may block that loop, but it does not make the resolver run on that loop or cause this failure. Restrict this warning to direct exporter execution so users are not told that normal flush/shutdown loses telemetry.
… caveat
In opentelemetry-sdk 1.39, force_flush() exports on the calling thread, so an
async resolver fails when it is called from a running event loop. shutdown()
runs its final export on the batch worker thread and only blocks the caller.
The README previously said both would fail.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Copilot's review of 1e83d75 listed one "previously missed" docs item: the hosting README said force_flush() and shutdown() both fail with an async resolver when called from an event loop. I checked it against the installed opentelemetry-sdk 1.39.1, and it's half right:
force_flush() exports on the calling thread. BatchProcessor.force_flush calls self._export(EXPORT_ALL) directly ("Blocking call to export"), and _EnrichingBatchSpanProcessor only overrides on_end. Called from a running event loop, the async resolver can't be awaited and that export fails with a logged error, so the README was right about this one.
shutdown() runs its final export on the worker thread (the _export(EXPORT_ALL) after the worker() loop). It only blocks the caller while it joins the worker, so the README was wrong about this one.
6b358f0 updates the README to describe each call separately. It's docs only; the code is unchanged.
TokenResolver permits any Awaitable, including an asyncio.Task or Future. In the active-event-loop failure path, only bare coroutine objects are closed, so a resolver-returned task keeps running after this export reports failure and may still mutate a token cache or raise an unobserved exception. Cancel standard futures/tasks before raising.
The reason will be displayed to describe this comment to others. Learn more.
PR Review Agent - approved
Re-reviewed 2b3710c..6b358f0, the unchanged PR contract, every author response and thread reply, the late exact-head Copilot review, and the approval gate over the full PR.
Author-fix adjudication
Accepted - direct runtime dependency:1e83d75 declares microsoft-agents-a365-runtime directly in the hosting package, preserves the repository's name-only internal dependency convention, and adds the matching uv.lock dependency and metadata edges. scripts/verify_constraints.py passes locally, and the exact-head dependency review, CodeQL, version, and changed-SDK checks pass.
Accepted - flush/shutdown documentation:6b358f0 now distinguishes OpenTelemetry 1.39.1 behavior correctly: BatchProcessor.force_flush() performs its blocking export on the caller, while shutdown's final export runs on the worker before the caller's join returns. The async-resolver guidance now matches both paths.
Accepted - all earlier fixes remain present: tuple identity isolation; entry-owned lock lifecycle and overflow trimming; standard transient retries; resolver typing; invalidation detachment; and cached-token snapshot return all remain source-verified with their discriminating regressions.
All four review threads are resolved, and no blocking or should-fix finding remains.
What I checked
Correctness / edge cases: the new direct dependency matches the production import and lock metadata; the docs-only follow-up matches the installed SDK's caller-thread and worker-thread export paths.
Test coverage: the dependency constraint verifier passes locally, and every exact-head check, including Python 3.11/3.12, passes.
Security / privacy: no new token, tenant-isolation, claim-validation, or logging behavior changed in this delta.
Pattern / blast radius: the package follows the workspace source and name-only dependency conventions; the fix is localized to published package metadata.
Verdict: approved. Merge remains subject to branch protection, completion of the pending Python jobs, and the PR's documented coordinated cross-SDK rollout order (with the CLI PR last).
When export runs inside an active event loop, the exporter can't await an
async token resolver and fails that export. It closed bare coroutines but left
a resolver-returned asyncio.Future or Task running, where it could still
mutate a token cache or raise an unobserved exception. Cancel those too.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Copilot's review of 6b358f0 listed a Medium "previously missed" item in _resolve_token: when export can't await an async resolver because an event loop is already running, only bare coroutines were closed. A resolver-returned asyncio.Future or Task kept running after the export reported failure.
Fixed in ddd0df1. That path now also cancels asyncio.Future instances, which covers Task too. The new test test_awaitable_resolver_in_active_event_loop_fails_and_is_released covers a coroutine, a future and a task. It failed before the fix, and now it passes 10 runs in a row. Full suite: 832 passed / 3 skipped, ruff clean.
This describes the BatchSpanProcessor.force_flush() threading model incorrectly. With the locked OpenTelemetry SDK, force_flush() signals the batch processor's worker and waits for it; the worker performs the export. Therefore, an event loop on the calling thread does not make the async resolver fail (although calling force_flush() directly still blocks that loop). Please document the worker-thread behavior so users do not infer a resolver restriction that does not exist.
Jason-R-Lien sorry, your 22:02Z approval on 6b358f0 was dismissed by my push of ddd0df1 about 90 seconds later. I pushed it before I saw your review.
ddd0df1 is the only change since your approval. It's a 2-line fix for Copilot's Medium "previously missed" item on 6b358f0: when export can't await an async resolver because an event loop is already running, the exporter now also cancels a resolver-returned asyncio.Future/Task, not just a bare coroutine. test_awaitable_resolver_in_active_event_loop_fails_and_is_released covers a coroutine, a future and a task, and it failed before the fix. All exact-head checks pass (Python 3.11 and 3.12 included), and the full suite gives 832 passed / 3 skipped.
Copilot's newest Low item, on ddd0df1, says force_flush() exports on the worker thread. That's a false positive for the locked opentelemetry-sdk 1.39.1, which matches what you verified: BatchProcessor.force_flush calls self._export(EXPORT_ALL) on the caller. So the README is unchanged, and I won't push anything more unless you ask for a change.
The reason will be displayed to describe this comment to others. Learn more.
PR Review Agent - approved
Re-reviewed 6b358f0..ddd0df1, the updated PR description, every author response and thread reply, the late exact-head Copilot review, and the approval gate over the full PR.
Author-fix adjudication
Accepted - release resolver futures/tasks:ddd0df1 now cancels asyncio.Future instances when export is invoked inside an active event loop; asyncio.Task inherits from Future, so the same branch releases both forms. The new regression covers a coroutine, future, and task and discriminates the prior head, which only closed bare coroutines.
Rejected - late force_flush() documentation finding: the locked OpenTelemetry SDK 1.39.1 implementation calls _export(EXPORT_ALL) directly from BatchProcessor.force_flush() ("Blocking call to export"). The README's caller-thread warning is therefore correct; the bot's worker-thread claim does not apply to this version.
Accepted - all earlier fixes remain present: tuple identity isolation; bounded entry-owned refresh locks; transient exception retries; resolver typing; invalidation detachment; cached-token snapshot return; direct runtime dependency metadata; and separate force_flush()/shutdown() guidance remain source-verified.
All four review threads are resolved, no blocking or should-fix finding remains, and all exact-head checks pass, including Python 3.11/3.12, CodeQL, dependency review, versioning, and CLA.
What I checked
Correctness / edge cases: pending coroutine, future, and task cleanup; already-established active-loop failure behavior; and the prior-head counterfactual.
Test coverage: the new test exercises all three awaitable shapes and verifies no HTTP request is sent. A local targeted run could not restore build dependencies because PyPI TLS negotiation failed; exact-head CI completed successfully.
Security / privacy: cleanup does not alter token values, authorization routing, identity partitioning, or logging.
Performance / COGS: cancellation prevents abandoned resolver work; no new polling, storage, telemetry volume, or paid dependency.
Pattern / blast radius: the change is localized to the existing awaitable rejection path and preserves the exporter contract.
Verdict: approved. Merge remains subject to branch protection and the PR's coordinated rollout gate: ship the related SDK, CLI, samples, and skills changes together, with the CLI PR last.
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.
Jason-R-Lien
left a comment
Summary
This is the Python port of microsoft/Agent365-nodejs#290. Agent 365 observability (OBS) export now uses only the app-only S2S route, so customers no longer need to admin-consent the delegated
Agent365.Observability.OtelWritepermission for telemetry.build_export_url()always returns/observabilityService/tenants/{tenantId}/otlp/agents/{agentId}/traces?api-version=1.Agent365ExporterOptions.use_s2s_endpointis deprecated and ignored, even whenFalse./observability. HTTP 401, 403 and 404 fail the batch without a retry or a delegated route.Authorizationheader. Resolver exceptions still fail the group.Agent365ExporterOptionsalready declared) are now awaited when the export runs outside an active event loop, such as theBatchSpanProcessorworker. Previously the coroutine object ended up in theAuthorizationheader.exchange_token.AgenticTokenCache.refresh_observability_token(agent_id, tenant_id, token_resolver):(agent_id, tenant_id), keyed by the tuple so IDs that contain a colon can't collide;invalidate_all()reclaim it; eviction skips an entry whose refresh is still running, and the next insertion trims any overflow;expclaim after a fallback TTL of 1 h;invalidate_token(), so a refresh still in flight can't repopulate it.get_observability_token()keeps its async signature. It is now a pure cache read that returns the cached app-only token, orNone, and never exchanges a token.register_observability(...)now logs once and does nothing. It never callsAuthorization.exchange_token.async def token_resolver(agent_id, tenant_id): return await cache.refresh_observability_token(agent_id, tenant_id, acquire_app_only_obs_token). The hosting README documents the constraints for async resolvers on the exporter thread./.default.get_observability_authentication_scope()now returnsapi://9b975845-388f-4429-889e-eab1ef63949c/.default, which is what client credentials requires. This matchesPROD_OBSERVABILITY_SCOPEin the Node SDK.versioning/TARGET-VERSIONgoes from1.0.0to2.0.0.Compatibility
This is a breaking change, hence the major version bump:
scptokens.register_observabilitytogether withawait get_observability_tokenmust switch torefresh_observability_tokenwith an app-only resolver. Until it does, those calls don't raise: they log the migration error once and returnNone, so telemetry stops but the app's request path keeps working.use_s2s_endpoint=True. They work unchanged on this version.The SDK does not validate token claims itself. Resolvers should check that the token is app-only:
idtyp=app. Whenidtypis absent, accept a non-emptyrolesarray oroid == sub.scp.audand expiry.This is documented in the core and hosting READMEs.
Coordinated export configuration (CLAUDE.md)
This PR changes two of the three constants that must stay in sync. I reviewed all three together:
PROD_OBSERVABILITY_SCOPEapi://9b975845-388f-4429-889e-eab1ef63949c/.default.../Agent365.Observability.OtelWritescopeDEFAULT_ENDPOINT_URLhttps://agent365.svc.cloud.microsoftbuild_export_url()path/observabilityService/tenants/{tenantId}/otlp/agents/{agentId}/traces?api-version=1/observability/...unlessuse_s2s_endpoint=Truetest_export_config_consistency.pyis updated to the new values. The live check below uses a token for this scope, sent to this host and path.Validation
ruff checkandruff format --checkpass.uv run --frozen pytest tests/ -m "not integration"gives 832 passed, 3 skipped and 9 deselected. The baseline on main is 800 passed and 3 skipped. The delegated-exchange cache tests are replaced by tests for the app-only resolver./.defaultscoperegister_observabilitylogs once and does nothingTimeoutErrorandConnectionResetErrorare retriedinvalidate_token()TypeErrorget_observability_tokenstays asyncclient_credentialsgrant for the OBS/.defaultscope. The token hasidtyp=app, no app roles and noscp, so no OtelWrite grant is involved.invoke_agentspan was exported through the SDK exporter, in the workspace environment, in four configurations:POST /observabilityService/tenants/{t}/otlp/agents/{a}/traces?api-version=1→ 200,SUCCESSuse_s2s_endpoint=False(deprecated)SUCCESSFAILURE, no HTTP request sent, error loggedAgenticTokenCache.refresh_observability_token, two exportsapi://9b975845-388f-4429-889e-eab1ef63949c/.default. The cache was reused across the exporter's separate event loops.Merge gate
Ship together with the other PRs for this change:
Merge the CLI PR last.