Add configurability to set tags and metadata on langfuse traces#53
Conversation
34b8135 to
bc14b93
Compare
Review NotesOverallGood architecture — the 1. Bug workflow nodes are not coveredThe PR only enriches context in feature workflow nodes ( The bug workflow nodes that invoke the agent were not updated:
If someone configures 2. The per-node enrichment approach is fragileThe current design requires every node that calls the agent to manually build a ~6-line context dict: context = {
"ticket_key": ticket_key,
"ticket_type": state.get("ticket_type", ""),
"current_node": state.get("current_node", ""),
"event_type": state.get("event_type", ""),
"event_source": state.get("context", {}).get("source", ""),
"retry_count": state.get("retry_count", 0),
}This is copy-pasted into 7 files and will need to be added to every future node that calls the agent. If someone forgets (as happened with the bug workflow nodes), traces from that node get no tags/metadata. Suggested alternative: Resolve the trace fields once in the orchestrator worker — it already has the full workflow state at invocation time — and store the resolved
3. Field naming inconsistency
|
bc14b93 to
1bcdf07
Compare
Addresses review feedback from forge-sdlc#53: - Resolve tags/metadata once in the worker and pass via state/config, eliminating copy-pasted per-node context dicts across 7 feature nodes - Extend coverage to ContainerRunner-based bug workflow nodes (triage, rca_analysis, plan_bug_fix) so all nodes get trace enrichment - Fix TracingField naming to match state keys (WORKFLOW_STEP → CURRENT_NODE, etc.) for consistency between config names, Langfuse keys, and state keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
Addresses review feedback from forge-sdlc#53: - Resolve tags/metadata once in the worker and pass via state/config, eliminating copy-pasted per-node context dicts across 7 feature nodes - Extend coverage to ContainerRunner-based bug workflow nodes (triage, rca_analysis, plan_bug_fix) so all nodes get trace enrichment - Fix TracingField naming to match state keys (WORKFLOW_STEP → CURRENT_NODE, etc.) for consistency between config names, Langfuse keys, and state keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
9da99f6 to
6eb63de
Compare
Addresses review feedback from forge-sdlc#53: - Resolve tags/metadata once in the worker and pass via state/config, eliminating copy-pasted per-node context dicts across 7 feature nodes - Extend coverage to ContainerRunner-based bug workflow nodes (triage, rca_analysis, plan_bug_fix) so all nodes get trace enrichment - Fix TracingField naming to match state keys (WORKFLOW_STEP → CURRENT_NODE, etc.) for consistency between config names, Langfuse keys, and state keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
6eb63de to
2b787f3
Compare
|
Hi @eshulman2 thank you for the feedback. I've push a number of changes that address all of your concerns. Please let me know if you have questions. |
Addresses review feedback from forge-sdlc#53: - Resolve tags/metadata once in the worker and pass via state/config, eliminating copy-pasted per-node context dicts across 7 feature nodes - Extend coverage to ContainerRunner-based bug workflow nodes (triage, rca_analysis, plan_bug_fix) so all nodes get trace enrichment - Fix TracingField naming to match state keys (WORKFLOW_STEP → CURRENT_NODE, etc.) for consistency between config names, Langfuse keys, and state keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
2b787f3 to
8e82dcb
Compare
Addresses review feedback from forge-sdlc#53: - Resolve tags/metadata once in the worker and pass via state/config, eliminating copy-pasted per-node context dicts across 7 feature nodes - Extend coverage to ContainerRunner-based bug workflow nodes (triage, rca_analysis, plan_bug_fix) so all nodes get trace enrichment - Fix TracingField naming to match state keys (WORKFLOW_STEP → CURRENT_NODE, etc.) for consistency between config names, Langfuse keys, and state keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
8e82dcb to
3c11e12
Compare
Claude Code reviewAll three concerns from the original review are addressed: bug workflow nodes are now covered via the orchestrator worker, per-node context boilerplate is eliminated, and Found 2 issues in the refactored code:
forge/src/forge/integrations/langfuse/fields.py Lines 275 to 286 in 3c11e12
forge/src/forge/integrations/agents/agent.py Lines 1011 to 1020 in 3c11e12 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Addresses review feedback from forge-sdlc#53: - Resolve tags/metadata once in the worker and pass via state/config, eliminating copy-pasted per-node context dicts across 7 feature nodes - Extend coverage to ContainerRunner-based bug workflow nodes (triage, rca_analysis, plan_bug_fix) so all nodes get trace enrichment - Fix TracingField naming to match state keys (WORKFLOW_STEP → CURRENT_NODE, etc.) for consistency between config names, Langfuse keys, and state keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
3c11e12 to
ede1ba3
Compare
Addresses review feedback from forge-sdlc#53: - Resolve tags/metadata once in the worker and pass via state/config, eliminating copy-pasted per-node context dicts across 7 feature nodes - Extend coverage to ContainerRunner-based bug workflow nodes (triage, rca_analysis, plan_bug_fix) so all nodes get trace enrichment - Fix TracingField naming to match state keys (WORKFLOW_STEP → CURRENT_NODE, etc.) for consistency between config names, Langfuse keys, and state keys Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Dan Childers <dchilder@redhat.com>
b47dbef to
27e6c43
Compare
|
Hi @eshulman2 - thank you for the feedback. I made the changes and also made a few edits to .env.example to include better documentation and make sure the new env vars are empty by default |
eshulman2
left a comment
There was a problem hiding this comment.
the last thing that is missing IMHO is docs, please update the config reference and the developer guide with information regarding this change
|
Good idea, @eshulman2 - I just added documentation as described and pushed the changes. Please let me know what you think |
eshulman2
left a comment
There was a problem hiding this comment.
I noticed you removed some context from some nodes, why is that?
| task="sync-pr-description", | ||
| prompt=prompt, | ||
| context={"owner": owner, "repo": repo, "pr_number": pr_number}, | ||
| context={ |
There was a problem hiding this comment.
any reason for this change?
|
I believe this might give some context on the issues I mentioned in the review. The trace context should not be merged into the agent system prompt. Agent prompts and skills should be isolated from the tags/metadata we collect for observability. The problem is in The merge was introduced because Suggested fix: keep the two concerns separate in
# Domain context → system prompt only
if context:
system_prompt += "\n\nContext:\n"
for key, value in context.items():
if value is not None:
system_prompt += f"- {key}: {value}\n"
# Trace context → Langfuse only, never touches the prompt
trace_state = {**get_trace_context(), "system_prompt_length": len(system_prompt), "llm_model": ...}
trace_tags, trace_metadata = resolve_trace_fields(trace_state)Nodes that need |
|
A few other things worth addressing:
The
ContextVar is never reset after workflow completion. Duplicate fields are silently accepted. |
- Remove inline "system_prompt_length" - Create the mechanism to pass node state to context to allow writing configured metadata and traces to langfuse traces Signed-off-by: Dan Childers <dchilder@redhat.com> Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
3434028 to
1a9a736
Compare
…prompts Grafana dashboards: - Port forge-business, forge-engineering, forge-issue-detail dashboards into devtools/grafana/dashboards/ - Add datasource provisioning for ClickHouse (Langfuse), Prometheus, Redis - Add compose.grafana.yml (standalone) and compose.langfuse-network.yml (optional overlay for self-hosted Langfuse) compose files - Wire Grafana into docker-compose.yml and devtools/docker-compose.dev.yml - Document required LANGFUSE_TRACE_TAGS/METADATA values for dashboard queries in .env.example, config reference, and developer guide - Clarify compose.langfuse-network.yml as opt-in: running it without Langfuse up fails the whole stack; base commands now work without it Trace context fix: - Add trace_context parameter to ForgeAgent.run_task() — fields passed there go to Langfuse only and are never written to the system prompt - Revert generate_prd/spec/epics/regenerate/answer_question to original minimal prompt contexts; workflow state trace fields forwarded via trace_context instead - Fix sync_pr_description in code_review.py the same way Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Summary
LANGFUSE_TRACE_TAGSandLANGFUSE_TRACE_METADATAenv varsdevtools/grafana/with full provisioningChanges
Langfuse trace configurability
LANGFUSE_TRACE_TAGS/LANGFUSE_TRACE_METADATA: comma-separated field lists that control what workflow context is attached to Langfuse tracesticket_key,ticket_type,project_id,workflow_step,repo,pr_number,ci_status,event_source,event_type,llm_model,retry_count,system_prompt_lengthGrafana dashboard stack
forge-business(ticket throughput, cycle time),forge-engineering(LLM latency, token usage),forge-issue-detail(per-ticket trace drill-down)devtools/grafana/compose.grafana.yml: standalone Grafana compose for dashboard devdevtools/grafana/compose.langfuse-network.yml: optional overlay that joins Grafana to a self-hosted Langfuse Docker network (requires Langfuse to be running — omit this file if not using self-hosted Langfuse)docker-compose.ymlanddevtools/docker-compose.dev.ymlTrace context leakage fix (prompted by review feedback)
trace_contextparameter toForgeAgent.run_task()— fields passed there are forwarded to Langfuse only and never written to the system promptgenerate_prd,generate_spec,generate_epics,regenerate_with_feedback,answer_questionto their original minimal prompt contexts; workflow state trace fields go viatrace_contextinsteadsync_pr_descriptionincode_review.pythe same waycurrent_node,event_type,retry_countwere leaking verbatim into the agent system prompt for tasks where they're irrelevantDocs
docs/reference/config.md: added Langfuse trace field and Grafana variable referencedocs/developer-guide.md: added trace tags/metadata section and Grafana setup instructions.env.example: documented new vars with recommended values for dashboard compatibilityTest plan
uv run pytest tests/unit/ -qpassesdocker compose --env-file .env -f docker-compose.yml up -d prometheus grafana— Grafana reachable at http://localhost:3010-f devtools/grafana/compose.langfuse-network.yml— ClickHouse datasource connects and dashboard panels render🤖 Generated with Claude Code