fix(sdk): record invocation parameters on the root span#4423
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughResolverMiddleware now imports TracingContext and mirrors the resolved request.data.parameters into TracingContext.get().parameters before passing control to the next middleware. Tests run the middleware inside a tracing context and assert TracingContext.get().parameters for direct, revision-derived, reference-hydrated, and ChangesResolver Middleware Parameter Tracing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py (1)
347-390: ⚡ Quick winAdd a dedicated references-hydration tracing test.
This test seeds
data.revisiondirectly, so it bypasses theneeds_reference_hydration/resolve_referencespath. Please add one case withrequest.references(and nodata.revision) to lock the regression path this PR calls out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 704d5a4c-4385-4189-b95c-d55cf8856d89
📒 Files selected for processing (2)
sdks/python/agenta/sdk/middlewares/running/resolver.pysdks/python/oss/tests/pytest/utils/test_resolver_middleware.py
| # records them under ag.meta.configuration. This covers both cases: parameters | ||
| # supplied directly in the invoke payload, and parameters fetched from a | ||
| # revision via references. It also reflects any @ag.embed expansion above. | ||
| TracingContext.get().parameters = request.data.parameters |
There was a problem hiding this comment.
# Mirror the resolved parameters onto the tracing context so the root span
# records them under ag.meta.configuration. This covers both cases: parameters
# supplied directly in the invoke payload, and parameters fetched from a
# revision via references. It also reflects any @ag.embed expansion above.
f1ffa60 to
d2454aa
Compare
Symptom: invoking a chat or completion app from the playground produced
traces with ag.meta.configuration = {} on the root span, even though the
invoke payload carried a full parameters object (prompt, llm_config, etc).
The handler still received the parameters and the call ran correctly. Only
the trace was empty.
Cause: the tracing decorator at decorators/tracing.py:393 reads
TracingContext.parameters when setting the root span's configuration
attribute. That field exists on TracingContext but no code in the SDK
ever assigned to it. The grep was empty across the repo. So the root
span always recorded the fallback {}.
Fix: have ResolverMiddleware mirror the resolved parameters onto the
tracing context after it merges request and revision values. The resolver
is the single funnel where both paths converge (parameters supplied in the
invoke payload, or parameters fetched from a revision via references) and
it runs after @ag.embed expansion. Whatever the handler is about to
receive is what the trace will record.
Adds three regression tests in TestResolverMiddlewareTracingParameters
covering each path. Also wraps the existing TestResolverMiddlewareEmbedGate
tests in tracing_context_manager to match how workflow.invoke runs the
middleware in production. Without the wrapper the mutation lands on a
throwaway default context and silently passes.
d2454aa to
a2c4171
Compare
Symptom
Invoking a chat or completion app from the playground produced traces with
ag.meta.configuration = {}on the root span, even though the invoke payload carried a fullparametersobject (prompt, llm_config, etc).The handler still received the parameters and the call ran correctly. Only the trace was empty.
Cause
The tracing decorator at
sdks/python/agenta/sdk/decorators/tracing.py:393-397readsTracingContext.parameterswhen setting the root span's configuration attribute:TracingContext.parametersis declared as a field insdks/python/agenta/sdk/contexts/tracing.py:15, but no code in the SDK ever assigned to it. A grep across the repo for any writer was empty. So the root span always recorded the{}fallback.The
workflow.invokemethod populatestracing_ctx.credentials,flags,tags,meta,references, andlinks(decorators/running.py:346-356), but notparameters. Onlyrunning_ctx.parametersis set (line 374), and the tracing decorator does not read fromRunningContext.Fix
ResolverMiddlewaremirrors the resolved parameters onto the tracing context after it merges request and revision values:The resolver is the single funnel where both invocation paths converge:
It also runs after
@ag.embedexpansion. So whatever the handler is about to receive is what the trace records.Test plan
pytest sdks/python/oss/tests/pytest/utils/test_resolver_middleware.py -v(22 passed)TestResolverMiddlewareTracingParameterscover the three resolver paths: request-supplied, revision-fetched, and post-embed-expansionTestResolverMiddlewareEmbedGatetests now wrap the middleware intracing_context_managerto mirror production usage. Without the wrapper, the mutation would land on a throwaway default context and silently pass even if the assignment regressed.ag.meta.configurationpopulated with the prompt and llm_config