E2E test improvements: observability, metrics, Opus 4.8 support - #556
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the end-to-end triage agent tests to support the NOT_AFFECTED resolution. It introduces a new test case using NotAffectedData and restructures the assertions in test_triage_agent to be resolution-specific. Feedback suggests strengthening the assertions for the NOT_AFFECTED resolution by verifying the data type is NotAffectedData and ensuring justification_category is present.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| elif expected_output.resolution == Resolution.NOT_AFFECTED: | ||
| # TODO: validate justification_category against a known set of values | ||
| assert real_output.data.explanation |
There was a problem hiding this comment.
To ensure robust validation of the NOT_AFFECTED resolution, we should explicitly verify that real_output.data is an instance of NotAffectedData (preventing Pydantic union coercion issues) and assert that justification_category is populated and non-empty, as it is a required metadata field for Red Hat's triage process.
| elif expected_output.resolution == Resolution.NOT_AFFECTED: | |
| # TODO: validate justification_category against a known set of values | |
| assert real_output.data.explanation | |
| elif expected_output.resolution == Resolution.NOT_AFFECTED: | |
| assert isinstance(real_output.data, NotAffectedData) | |
| assert real_output.data.explanation | |
| assert real_output.data.justification_category |
904d033 to
36c7674
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the observability and logging of the agents and the trace server. Key changes include adding structured logging to the trace server, expanding the metrics middleware to track token usage and tool calls, wrapping E2E test workflows with JIRA issue tracing contexts, and adding a LiteLLM provider ID workaround for Vertex AI. The review feedback highlights opportunities to make token extraction in the metrics middleware more robust against dictionary payloads, remove a duplicate assertion in the triage tests, implement the documented flushing and shutdown behavior in the OpenTelemetry fixtures, and correct the framework logger name to properly suppress verbose logs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Claude
otherwise we get a flood of logs Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Claude Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
instead of running all by default Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Claude
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Cursor
tokens, tool calls Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Claude
Wrap each test case's run_workflow() call with _span_processor.jira_issue_context(issue) so that emitted OTEL spans carry the jira.issue attribute. Without this, the trace-server cannot index spans by issue key. Both triage and backport e2e tests are updated: - observability_fixture now yields the AgentSpanProcessor and stores it in a module-level _span_processor for test cases to use - TracerProvider is explicitly flushed and shut down at teardown since the container may be killed before atexit handlers run Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Claude
it's really useful to investigate later Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Claude
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
Beeai uses "vertexai" as the litellm provider ID, but litellm only recognizes "vertex_ai" for adaptive thinking detection (Claude 4.6+). Without this, litellm sends thinking.type=enabled which opus-4-8 rejects. Signed-off-by: Tomas Tomecek <ttomecek@redhat.com> Assisted-by: Claude
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
| [ | ||
| test_case.jira_issue, | ||
| m.get("agent_name", ""), | ||
| f"{m.get('duration', 0):.0f}s", | ||
| m.get("tool_calls", 0), | ||
| m.get("prompt_tokens", 0), | ||
| m.get("completion_tokens", 0), |
| .PHONY: run-triage-agent-e2e-tests | ||
| run-triage-agent-e2e-tests: | ||
| $(COMPOSE) -f $(COMPOSE_FILE) --profile=e2e-test run --rm \ | ||
| $(COMPOSE) -f $(COMPOSE_FILE) --profile=e2e-test run \ |
There was a problem hiding this comment.
is there some case where we need it cleaned up? Should we document it?
There was a problem hiding this comment.
I'm assuming that @TomasKorbar just disliked these containers to linger after a test run and that he had to manually remove them.
For me it's exact opposite, I used them to review the run and access the logs - the --rm was a blocker for me.
nforro
left a comment
There was a problem hiding this comment.
LGTM. You use both Claude and Cursor?
I used Cursor for one of the commits because I had it opened in an IDE: the logging addition to trace-server. Wanted to give it a try, it did okay but for other work it was poor so I switched back to Claude Code. |
Summary
Batch of E2E test infrastructure improvements:
jira.issueviaAgentSpanProcessorcontext so the trace-server can index spans by issue key; flush TracerProvider explicitly at teardownvertexai→vertex_ai) so adaptive thinking detection works for Claude 4.6+ymir/andscripts/directoriespackage/fix_versionare only checked for BACKPORT/REBASE resolutionsTest plan
make run-triage-agent-e2e-testspassesjira.issueattribute in trace-server🤖 Generated with Claude Code