| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…ies (1:1) Previously _begin_obs skipped the obs wrapper for ANY Temporal activity (Option A) and only stamped the ambient RunActivity span, so all business spans in a turn collapsed onto ONE obs span (52:1). But inside a *business* activity, start_span and end_span run in the SAME process, so a wrapper is safe there. Option A is only required for the SDK's own dispatched START_SPAN/END_SPAN activities (the in_temporal_workflow path), where start and end are separate activities on possibly different workers. Discriminate on activity type: _in_tracing_dispatch_activity() is true only for the "start-span"/"end-span" activities. For everything else (sync, or a business activity) open a real per-step wrapper — it nests under the interceptor's ambient RunActivity span and closes in-process, giving each business span its own obs span (1:1), matching the sync path. The bounded _OBS_HANDLES registry backstops any mis-discrimination.
…tivities
In dd_only (the default), the wrapper branch of _begin_obs read ddtrace for
both open_obs_span and the obs_correlation fallback. Inside a Temporal worker
there is no ddtrace request context, so open_obs_span returned None and the
fallback obs_correlation() returned {} -- every business span in an async turn
persisted with no obs_trace_id/obs_span_id. The ambient span in an activity is
the temporalio OTel TracingInterceptor span regardless of SGP_OBS_MODE (same
reasoning already applied to the dispatch/tag branch).
Fix: thread prefer_otel through the wrapper branch, keyed on "in any Temporal
activity" (restored _in_temporal_activity). open_obs_span gains a prefer_otel
param that opens an OTel wrapper first (falling back to ddtrace). This also
restores real per-step OTel wrapper spans in dd_only business activities, not
just the ids.
Adds a dd_only-inside-a-business-activity regression test (asserts OTel ids win
over ddtrace, not empty); the earlier verification ran in lgtm so it missed this.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The prior tests stubbed _in_tracing_dispatch_activity wholesale, so the actual
`activity.info().activity_type in ("start-span", "end-span")` comparison -- the
one line preventing a cross-worker handle leak inside START_SPAN -- had no
coverage. Add tests that fake activity.info():
- start-span / end-span -> True (asserted against TracingActivityName.value, so
this fails if the enum ever drifts from the strings hardcoded in trace.py),
- a business activity type (process_mortgage_turn) -> False,
- not in an activity -> False (in_activity guard short-circuits before info()),
plus a small _in_temporal_activity() truth-table test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Compare activity.info().activity_type against TracingActivityName.START_SPAN / END_SPAN instead of the string literals "start-span" / "end-span", so the check can't silently drift from the enum that actually names the activities (@activity.defn(name=TracingActivityName.START_SPAN)). Uses a lazy import inside the function to avoid the activities -> TracingService -> AsyncTracer -> trace import cycle (the reason literals were used originally); at call time, inside an activity, the module graph is fully loaded so the import is safe. activity_type round-trips as the enum's str value, which a str-Enum member compares equal to. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…emporal_activity
Deleting _in_temporal_activity earlier dropped the TODO it carried. This PR does
item (a) of it (the named per-step wrapper); the other two are still open, so
re-home them on _begin_obs where the per-step wrapper decision now lives:
(1) TurnTrace RETRY/ASYNC roll-up -- retried turns currently surface as N
per-attempt span sets, not one PRIMARY + N RETRY view.
(2) multi-replica bounded-_OBS_HANDLES + obs_trace_id-resolves-to-turn check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… drift prefer_otel read like a per-call override of SGP_OBS_MODE, which undercut the mode as the source of truth. Rename to expect_otel: it is not a bypass but a context-derived expectation -- "the expected backend here is OTel" -- true on the Temporal path, where the interceptor span is OTel regardless of mode. Keep the mode authoritative and make config-vs-reality drift observable instead of silently absorbing it: warn_on_backend_drift() logs once (deduped per direction) when the expected backend has no active span but the other one does (e.g. dd_only configured but the live span is OTel). Fail-open -- it only warns; the caller still reads and falls back, so no correlation is lost. Wired into _begin_obs on both the dispatch and business-activity paths. Tests: the rename, plus two drift tests (warns once on mismatch; silent when the expected backend is the live one). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… guard The enum import sat above the in_activity() check, so it ran on every start_span -- including the pure-sync ACP path that never touches Temporal. That pulled the whole temporal activities module graph into workflow-less processes, made the docstring's "inside an activity" safety justification untrue for the path actually taken, and meant a broken import would silently return False (disabling dispatch discrimination) even on the sync path. Move it below the guard: sync never runs it, and inside an activity the graph is loaded so the lazy import stays safe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ift; rewrap docstring - warn_on_backend_drift probes the expected backend first and returns early when it is live, so the healthy common path skips the second probe -- and avoids re-attempting the import of a backend that isn't installed (failed imports aren't cached in sys.modules, so the finder cost otherwise recurs every span). Behavior is unchanged. - Rewrap the tag_ambient_obs_span docstring paragraph a prior edit left with one overlong line. Addresses review nits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| Back | FazBrowse Home | New Git URL |
What & why
Two ways to give a business step an obs presence on the Temporal path:
Before this change, _begin_obs used Option A for any Temporal activity, so every business span in an async turn collapsed onto one obs span, whereas the sync path gives each step its own (1:1). This implements the _in_temporal_activity follow-up called out in #484.
The fix
Option A is only actually required for the SDK's own dispatched start-span / end-span activities (the in_temporal_workflow() path), where start and end run as separate activities that Temporal can route to different workers — so a wrapper opened in start-span could never be closed by end-span (it would leak and its obs_span_id would dangle).
Inside a business activity (an agent turn's own adk.tracing.span, e.g. process_mortgage_turn), start and end run in the same process, so a wrapper is safe there: it nests under the interceptor's ambient RunActivity span and closes in-process.
_begin_obs now discriminates on activity type via _in_tracing_dispatch_activity() (true only for start-span/end-span):
The now-dead _in_temporal_activity() is removed (its TODO is exactly what this implements). The bounded _OBS_HANDLES registry backstops any mis-discrimination.
Why it matters
With per-step wrappers on the async path you can tell which hop belongs to which step (hops nest under their step, not one giant activity span), each step's persisted obs_span_id points at its own step-named span instead of a shared generic one, and async finally produces the same trace shape as sync.
Span volume on retries
A retried business activity now emits a full per-step wrapper set per attempt, each nested under that attempt's RunActivity (previously they collapsed onto the ambient activity span). This is the intended 1:1 shape, but until the TurnTrace roll-up lands a retried turn surfaces as N per-attempt span sets rather than one PRIMARY + N RETRY view. That roll-up and a multi-replica bounded-_OBS_HANDLES check are tracked as TODO(obs-followup) in trace.py.
Verified on infra-staging (rocket-mock-async-agent), one async turn
Each step (mortgage.advisor.turn, classify_intent, retrieve_docs.kb_query, authz.check.*, tool.*, telemetry.shard.*, synthesis.draft_reply) now gets its own obs span nested under RunActivity, matching the sync path.
Tests
Updated test_temporal_obs_backend.py for the new discriminator, plus two tests asserting: inside a dispatch activity → tag ambient (no wrapper); otherwise → open a wrapper.
🤖 Generated with Claude Code
Greptile Summary
The PR gives business steps inside Temporal activities dedicated observability wrapper spans while retaining ambient-span tagging for the SDK’s separately dispatched tracing activities.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Begin business span] --> B{Tracing dispatch activity?} B -->|Yes| C[Tag ambient Temporal OTel span] C --> D[Persist ambient correlation IDs] B -->|No| E{Inside any Temporal activity?} E -->|Yes| F[Open per-step OTel wrapper] E -->|No| G[Open wrapper selected by configured mode] F --> H[Persist wrapper correlation IDs] G --> H H --> I[Register handle] I --> J[End business span] J --> K[Close and remove wrapper handle]Reviews (8): Last reviewed commit: "refactor(tracing): probe expected backen..." | Re-trigger Greptile