add name attribute to FunctionBaseConfig for workflow naming in span exporter#1482
Conversation
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config (workflow/function)
participant Builder as Builder
participant Runtime as Runner
participant Tracing as Tracing/Exporter
Config->>Builder: provide function config (may include `name`)
Builder->>Builder: determine `instance_name`
note right of Builder: prefer provided instance_name\nif == WORKFLOW_COMPONENT_NAME use config.name → config.type
Builder->>Runtime: supply Function with `instance_name` and config
Runtime->>Runtime: resolve `workflow_name`
note right of Runtime: prefer config.name → instance_name\nif instance_name == WORKFLOW_COMPONENT_NAME then use config.type\nelse allow None
Runtime->>Tracing: emit WORKFLOW_START with resolved `workflow_name`
Runtime->>Tracing: emit WORKFLOW_END with resolved `workflow_name`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/nat/data_models/function.py`:
- Around line 27-38: Update the docstring in the Function configuration class to
wrap code entities in backticks: change occurrences of name and middleware to
`name` and `middleware`, and wrap the literal `middleware` section reference and
`YAML` if mentioned, so all code/config entities are backticked; locate the
docstring above the Field declaration for the name attribute (and the comment
block describing middleware) and apply the backticks consistently for `name`,
`middleware`, and any inline YAML section references.
🧹 Nitpick comments (1)
src/nat/runtime/runner.py (1)
168-180: Deduplicate workflow name resolution logic.The same fallback chain appears in both
resultandresult_stream. Centralizing it reduces drift and keeps behavior consistent.♻️ Suggested refactor
class Runner: + def _resolve_workflow_name(self) -> str: + """Resolve workflow name with backward-compatible fallback chain.""" + config = getattr(self._entry_fn, "config", None) + workflow_name = getattr(config, "name", None) + if not workflow_name: + workflow_name = getattr(self._entry_fn, "instance_name", None) + if workflow_name == WORKFLOW_COMPONENT_NAME: + workflow_name = getattr(config, "type", None) or "workflow" + elif not workflow_name: + workflow_name = "workflow" + return workflow_name @@ - config = getattr(self._entry_fn, 'config', None) - workflow_name = getattr(config, 'name', None) - if not workflow_name: - workflow_name = getattr(self._entry_fn, 'instance_name', None) - if workflow_name == WORKFLOW_COMPONENT_NAME: - workflow_name = getattr(config, 'type', None) or "workflow" - elif not workflow_name: - workflow_name = "workflow" + workflow_name = self._resolve_workflow_name() @@ - config = getattr(self._entry_fn, 'config', None) - workflow_name = getattr(config, 'name', None) - if not workflow_name: - workflow_name = getattr(self._entry_fn, 'instance_name', None) - if workflow_name == WORKFLOW_COMPONENT_NAME: - workflow_name = getattr(config, 'type', None) or "workflow" - elif not workflow_name: - workflow_name = "workflow" + workflow_name = self._resolve_workflow_name()Also applies to: 260-272
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
willkill07
left a comment
There was a problem hiding this comment.
Overall adding name makes sense. Would "display_name" be less likely to conflict with a user's existing configuration?
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
/ok to test 898d116 |
|
/ok to test 898d116 |
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/nat/runtime/test_runner_trace_ids.py`:
- Line 122: Remove the unnecessary `@pytest.mark.asyncio` decorator from the async
test in tests/nat/runtime/test_runner_trace_ids.py (the test currently annotated
with `@pytest.mark.asyncio`); leave the async def test_... function as-is since
pytest-asyncio auto-detects async tests, and run the test suite to confirm
behavior remains unchanged.
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
/ok to test 0695894 |
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
|
/ok to test 0b45e99 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/nat/runtime/test_runner_trace_ids.py`:
- Around line 141-145: The unused parameter to_type in the methods convert and
ainvoke should be explicitly marked as intentionally unused to silence Ruff
ARG002; inside each method (convert and ainvoke) add a simple no-op use of the
parameter (e.g., assign it to _ or an unused variable) with a brief comment like
"# silence ruff ARG002" so the signature stays the same but the linter sees the
parameter as used.
|
/ok to test 0b45e99 |
|
/merge |
Description
This PR adds the name attribute to FunctionBaseConfig, enabling workflows to be optionally named (non-breaking, backwards compatible). This allows developers to optionally name workflows and have those names carried through the span exporter and represented in OTEL data. This PR follows #1320 which enabled span parent-child lineage for nested tool calls.
/examples/observability/simple_calculator_observability/config-phoenix-nested.ymlhas been updated - showing this working in practice.Closes
#1335
By Submitting this PR I confirm:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.