feat: add OpenTelemetry tracing and metrics instrumentation - #1761
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces OpenTelemetry integration for distributed tracing and metrics observability throughout HolmesGPT. It adds a new OTel tracing module, updates the tracing factory with metrics state and auto-detection, integrates tracing into tool execution and server request handling, includes comprehensive documentation and test coverage, and updates build dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Server Handler
participant Tracer as OTel Tracer
participant ToolExec as Tool Executor
participant LLM as LLM Service
participant Metrics as Metrics Recorder
participant Exporter as OTLP Exporter
Client->>Server: POST /chat (request)
Server->>Tracer: start_trace(investigation)
activate Tracer
Tracer-->>Server: investigation_span
Server->>Metrics: investigation_count.add()
loop Investigation Loop
Server->>ToolExec: execute tool (child span)
activate ToolExec
ToolExec->>Tracer: start_span(tool.X)
ToolExec->>Metrics: tool_call_count.add()
ToolExec->>Metrics: tool_call_duration.record()
Tracer-->>ToolExec: tool_span
deactivate ToolExec
Server->>LLM: call LLM (child span)
activate LLM
LLM->>Tracer: start_span(gen_ai.chat)
LLM->>Metrics: llm_input_tokens.add()
LLM->>Metrics: llm_call_duration.record()
Tracer-->>LLM: llm_span
deactivate LLM
end
Server->>Metrics: investigation_duration.record()
Server->>Tracer: end investigation_span
deactivate Tracer
Tracer->>Exporter: export spans & metrics (batch)
Exporter-->>Tracer: ack
Server-->>Client: response (streamed or complete)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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. 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
henrikrexed
force-pushed
the
feat/otel-pr-clean
branch
2 times, most recently
from
March 13, 2026 18:26
91faf5c to
d465f0e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
holmes/core/otel_tracing.py (2)
6-6: Potential circular import betweentracing.pyandotel_tracing.py.This file imports
DummySpanandSpanTypefromholmes.core.tracing, whiletracing.pyimportsOpenTelemetryTracerfrom this module. The circular dependency is currently resolved becausetracing.pyuses lazy imports insidecreate_tracer(), but this is fragile.Consider extracting
DummySpanandSpanTypeinto a separatetracing_base.pymodule to make the dependency graph clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/otel_tracing.py` at line 6, The import of DummySpan and SpanType from tracing.py creates a fragile circular dependency with OpenTelemetryTracer (imported by tracing.py via create_tracer()); to fix, extract DummySpan and SpanType into a new module (e.g., tracing_base.py), move their definitions there, update otel_tracing.py to import DummySpan and SpanType from tracing_base, and update tracing.py to import those same symbols from tracing_base (leaving create_tracer() lazy imports intact); ensure all references to DummySpan and SpanType in both OpenTelemetryTracer and create_tracer() now point to the new tracing_base module.
164-172: Renametypeparameter to avoid shadowing Python builtin.The
typeparameter shadows the Python builtintypefunction. While this works, it can cause confusion and is flagged by static analysis (Ruff A002).♻️ Proposed fix
- def set_attributes(self, name: Optional[str] = None, type: Optional[str] = None, span_attributes: Optional[Dict[str, Any]] = None) -> None: + def set_attributes(self, name: Optional[str] = None, span_type: Optional[str] = None, span_attributes: Optional[Dict[str, Any]] = None) -> None: if name: self._span.update_name(name) if span_attributes:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/otel_tracing.py` around lines 164 - 172, The set_attributes method defines a parameter named type which shadows the built-in type; rename that parameter (e.g., to span_type or attribute_type) in the set_attributes signature and its type annotation, update any internal uses in the method body (none beyond the signature here), and update all callers across the codebase and tests to use the new parameter name to avoid Ruff A002 warnings and confusion; ensure type hints and any docstrings are updated to match the new parameter name.holmes/core/tool_calling_llm.py (3)
377-377: Avoid function call in default argument.
DummySpan()is evaluated once at function definition time, so all callers without an explicittrace_spanshare the same instance. WhileDummySpanis likely stateless, this pattern is fragile. UseNoneas the default and instantiate inside the function.The same pattern appears on lines 396, 423, and 1008 (flagged by static analysis).
♻️ Proposed fix
def prompt_call( self, system_prompt: str, user_prompt: str, response_format: Optional[Union[dict, Type[BaseModel]]] = None, - trace_span=DummySpan(), + trace_span=None, request_context: Optional[Dict[str, Any]] = None, ) -> LLMResult: + if trace_span is None: + trace_span = DummySpan() messages = [Apply the same pattern to
messages_call,call, andcall_stream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` at line 377, Replace the default parameter trace_span=DummySpan() with trace_span=None in the function signatures (e.g., messages_call, call, call_stream and any other functions using DummySpan() as a default) and inside each function, if trace_span is None, set trace_span = DummySpan() before use; this ensures a fresh DummySpan instance per call and avoids sharing one instance across callers while preserving existing behavior.
496-507: Misleading metric counter name for output tokens.Line 504 adds completion (output) tokens to
otel_metrics.llm_input_tokens. The counter name is misleading since it records both input and output tokens. Consider renaming the counter tollm_tokensortoken_usageinotel_tracing.pyto better reflect its purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 496 - 507, The metric counter otel_metrics.llm_input_tokens is used for both input and completion tokens in tool_calling_llm.py (see uses of otel_metrics.llm_input_tokens.add for raw.prompt_tokens and raw.completion_tokens); rename this counter to a neutral name (e.g., otel_metrics.llm_tokens or otel_metrics.token_usage) in the metrics definition (otel_tracing.py) and update all references (including the adds for input and output and any metric creation/registration) so the output token add uses the new counter instead of llm_input_tokens, and update any attribute/tag keys if needed to preserve gen_ai_token_type distinctions.
897-898: Inconsistent status enum comparison.Line 897 compares
status.value == "error"(string), but line 816 compares directly againstStructuredToolResultStatus.ERROR(enum). Using the enum directly is more robust and consistent.♻️ Proposed fix
- if tool_call_result.result.status and tool_call_result.result.status.value == "error": + if tool_call_result.result.status == StructuredToolResultStatus.ERROR: otel_metrics.tool_call_errors.add(1, tool_attrs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 897 - 898, The status comparison uses a raw string ("error") instead of the enum; change the check to compare tool_call_result.result.status directly with the enum StructuredToolResultStatus.ERROR (i.e., replace status.value == "error" with status == StructuredToolResultStatus.ERROR) and ensure StructuredToolResultStatus is imported or referenced where tool_call_result and otel_metrics.tool_call_errors.add are used so the metric increment remains executed for the enum case.tests/test_otel_tracing.py (2)
26-28: Fragile access to private OTel SDK internals.Accessing
trace._TRACER_PROVIDER_SET_ONCE._doneis necessary for test isolation but could break with OpenTelemetry SDK updates. Consider adding a comment with the SDK version this was tested against, or checking for attribute existence.♻️ Proposed defensive check
# Reset the global provider guard so we can set a fresh provider per test - trace._TRACER_PROVIDER_SET_ONCE._done = False # type: ignore[attr-defined] + # Tested with opentelemetry-sdk 1.20.0; may need adjustment for other versions + if hasattr(trace, "_TRACER_PROVIDER_SET_ONCE") and hasattr(trace._TRACER_PROVIDER_SET_ONCE, "_done"): + trace._TRACER_PROVIDER_SET_ONCE._done = False # type: ignore[attr-defined] trace.set_tracer_provider(provider)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_otel_tracing.py` around lines 26 - 28, The test directly mutates the OTel SDK private internals trace._TRACER_PROVIDER_SET_ONCE._done which is fragile; update the block around trace._TRACER_PROVIDER_SET_ONCE._done and trace.set_tracer_provider(provider) to first check for the existence of _TRACER_PROVIDER_SET_ONCE and its _done attribute before setting it (fallback to a safe no-op if absent), and add a short comment noting the OpenTelemetry SDK version this workaround was validated against to aid future maintenance.
328-334: Weak test due to global state dependency.This test acknowledges dependency on test ordering in the comment. The assertion
result is None or hasattr(result, "llm_input_tokens")doesn't meaningfully validate behavior. Consider either:
- Mocking
get_metricsto ensure a known state- Using a fixture that resets the global metrics state
- Removing this test if the behavior is already covered elsewhere
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_otel_tracing.py` around lines 328 - 334, Test depends on global OTel state and currently uses a weak assertion; change the test to control the global state and assert deterministic behavior: import holmes.core.otel_tracing.get_metrics and the module-level metrics variable (or use the module's reset function if available) and either set that global to None before calling get_metrics and assert result is None, or use pytest monkeypatch to replace get_metrics with a controlled stub and assert the expected return; alternatively, replace the test with a fixture that resets the module-level metrics state before invocation so get_metrics deterministically returns None and assert that exact outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/opentelemetry.md`:
- Around line 130-142: The DQL code block containing the queries (e.g., the
lines starting with "timeseries avg(holmesgpt.tool.call.duration, default:0),
by: {holmesgpt_tool_name}" and the other queries like "timeseries
sum(gen_ai.client.token.usage...)" and "timeseries
percentile(holmesgpt.tool.call.duration, 95)...") needs a language identifier on
the opening fence; change the opening ``` to ```sql (or ```text) so the block
becomes ```sql ... ``` to ensure consistent syntax highlighting/formatting for
the Dynatrace DQL examples.
---
Nitpick comments:
In `@holmes/core/otel_tracing.py`:
- Line 6: The import of DummySpan and SpanType from tracing.py creates a fragile
circular dependency with OpenTelemetryTracer (imported by tracing.py via
create_tracer()); to fix, extract DummySpan and SpanType into a new module
(e.g., tracing_base.py), move their definitions there, update otel_tracing.py to
import DummySpan and SpanType from tracing_base, and update tracing.py to import
those same symbols from tracing_base (leaving create_tracer() lazy imports
intact); ensure all references to DummySpan and SpanType in both
OpenTelemetryTracer and create_tracer() now point to the new tracing_base
module.
- Around line 164-172: The set_attributes method defines a parameter named type
which shadows the built-in type; rename that parameter (e.g., to span_type or
attribute_type) in the set_attributes signature and its type annotation, update
any internal uses in the method body (none beyond the signature here), and
update all callers across the codebase and tests to use the new parameter name
to avoid Ruff A002 warnings and confusion; ensure type hints and any docstrings
are updated to match the new parameter name.
In `@holmes/core/tool_calling_llm.py`:
- Line 377: Replace the default parameter trace_span=DummySpan() with
trace_span=None in the function signatures (e.g., messages_call, call,
call_stream and any other functions using DummySpan() as a default) and inside
each function, if trace_span is None, set trace_span = DummySpan() before use;
this ensures a fresh DummySpan instance per call and avoids sharing one instance
across callers while preserving existing behavior.
- Around line 496-507: The metric counter otel_metrics.llm_input_tokens is used
for both input and completion tokens in tool_calling_llm.py (see uses of
otel_metrics.llm_input_tokens.add for raw.prompt_tokens and
raw.completion_tokens); rename this counter to a neutral name (e.g.,
otel_metrics.llm_tokens or otel_metrics.token_usage) in the metrics definition
(otel_tracing.py) and update all references (including the adds for input and
output and any metric creation/registration) so the output token add uses the
new counter instead of llm_input_tokens, and update any attribute/tag keys if
needed to preserve gen_ai_token_type distinctions.
- Around line 897-898: The status comparison uses a raw string ("error") instead
of the enum; change the check to compare tool_call_result.result.status directly
with the enum StructuredToolResultStatus.ERROR (i.e., replace status.value ==
"error" with status == StructuredToolResultStatus.ERROR) and ensure
StructuredToolResultStatus is imported or referenced where tool_call_result and
otel_metrics.tool_call_errors.add are used so the metric increment remains
executed for the enum case.
In `@tests/test_otel_tracing.py`:
- Around line 26-28: The test directly mutates the OTel SDK private internals
trace._TRACER_PROVIDER_SET_ONCE._done which is fragile; update the block around
trace._TRACER_PROVIDER_SET_ONCE._done and trace.set_tracer_provider(provider) to
first check for the existence of _TRACER_PROVIDER_SET_ONCE and its _done
attribute before setting it (fallback to a safe no-op if absent), and add a
short comment noting the OpenTelemetry SDK version this workaround was validated
against to aid future maintenance.
- Around line 328-334: Test depends on global OTel state and currently uses a
weak assertion; change the test to control the global state and assert
deterministic behavior: import holmes.core.otel_tracing.get_metrics and the
module-level metrics variable (or use the module's reset function if available)
and either set that global to None before calling get_metrics and assert result
is None, or use pytest monkeypatch to replace get_metrics with a controlled stub
and assert the expected return; alternatively, replace the test with a fixture
that resets the module-level metrics state before invocation so get_metrics
deterministically returns None and assert that exact outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43f9d11a-801f-4a4e-bba8-b9b233c0126d
📥 Commits
Reviewing files that changed from the base of the PR and between a95e060 and d465f0e4db364f72488c795c56b2beb74dddb1af.
📒 Files selected for processing (11)
Dockerfiledocs/reference/opentelemetry.mdholmes/core/otel_tracing.pyholmes/core/tool_calling_llm.pyholmes/core/tracing.pyholmes/main.pyholmes/plugins/prompts/_general_instructions.jinja2mkdocs.ymlpyproject.tomlserver.pytests/test_otel_tracing.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/tool_calling_llm.py (1)
577-590:⚠️ Potential issue | 🟡 MinorUnsupported custom-tool errors currently bypass tool metrics.
The early return on unsupported custom calls (Line 577–Line 590) skips the metrics block (Line 648–Line 655), so failed tool invocations are not counted/duration-tracked.
Also applies to: 648-655
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 577 - 590, The early return for unsupported custom tools in the tool call branch prevents the metrics/metrics-timing path from running; update the branch that constructs the ToolCallResult (using tool_to_call, ToolCallResult, StructuredToolResult, and ToolCallingLLM._log_tool_call_result) so that it also invokes the same metrics recording/timing logic used later (the metrics block around tool_span/enable_tool_approval) before returning—either by calling the common metrics helper used for normal tool failures or by moving the return after calling ToolCallingLLM._log_tool_call_result plus the metrics recording code so failed custom-tool invocations are counted and duration-tracked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/helm-publish.yml:
- Line 16: Update the CHART_REPO environment variable and any other occurrences
that point to the personal namespace so the workflow publishes to the project
OCI path; replace the value "ghcr.io/henrikrexed" with the correct project
registry path (e.g., "ghcr.io/holmesgpt/charts") wherever CHART_REPO is defined
and referenced (also fix the same values around the occurrences noted at lines
34-36), ensuring the workflow's CHART_REPO variable and all uses of it
consistently target the project's OCI repository.
In `@holmes/core/tool_calling_llm.py`:
- Around line 850-858: The per-iteration LLM child span is logging cumulative
token counts from stats (stats.prompt_tokens, stats.completion_tokens,
stats.total_tokens) so each gen_ai.chat span shows accumulated totals; modify
the logic around llm_span.log to compute and log per-iteration usage (e.g.,
compute delta_prompt = stats.prompt_tokens - prev_prompt_tokens, delta_output =
stats.completion_tokens - prev_completion_tokens, delta_total =
stats.total_tokens - prev_total or use per-iteration stats if available) and
then log those delta values along with "holmesgpt.iteration": i; update/track
prev_* variables before the next iteration.
In `@server.py`:
- Around line 319-331: Add precise type hints to _stream_with_trace_cleanup:
annotate storage as contextlib.AbstractContextManager[Any], stream_generator as
Generator[bytes, None, None] (or Iterator[bytes] if you prefer), req_info as
str, and trace_span as opentelemetry.trace.Span, and set the function return
type to Generator[bytes, None, None]. Also add the required imports (from typing
import Any, Generator; from contextlib import AbstractContextManager; from
opentelemetry.trace import Span) at the top of the file.
- Around line 433-471: The root investigation span (trace_span) created when
chat_request.trace_span is None must be ended on all exit paths; wrap the LLM
call, metric recording, and response construction in a try/finally so that in
the finally block you call trace_span.end() only when you created it (check
chat_request.trace_span is None and trace_span is not None). Ensure the finally
still re-raises any exception (i.e., don't swallow errors) after ending the span
so failures from ai.call(), otel_metrics operations, or later code do not leave
the span open.
---
Outside diff comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 577-590: The early return for unsupported custom tools in the tool
call branch prevents the metrics/metrics-timing path from running; update the
branch that constructs the ToolCallResult (using tool_to_call, ToolCallResult,
StructuredToolResult, and ToolCallingLLM._log_tool_call_result) so that it also
invokes the same metrics recording/timing logic used later (the metrics block
around tool_span/enable_tool_approval) before returning—either by calling the
common metrics helper used for normal tool failures or by moving the return
after calling ToolCallingLLM._log_tool_call_result plus the metrics recording
code so failed custom-tool invocations are counted and duration-tracked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cddec8dd-a380-4167-88d8-7bd0688c8b9d
📥 Commits
Reviewing files that changed from the base of the PR and between d465f0e4db364f72488c795c56b2beb74dddb1af and 3043d378c9ca0ad26e5138bf21ca14b63ff1ab87.
📒 Files selected for processing (6)
.github/workflows/helm-publish.ymlDockerfileholmes/core/tool_calling_llm.pyholmes/main.pypyproject.tomlserver.py
henrikrexed
force-pushed
the
feat/otel-pr-clean
branch
from
March 17, 2026 16:27
3043d37 to
2427177
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/tool_calling_llm.py (1)
577-591:⚠️ Potential issue | 🟠 MajorUnsupported custom tool calls bypass tool metrics emission.
The early return in Lines 577-591 exits before Lines 647-655, so this error path doesn’t emit
tool.call.count/duration/errors.Also applies to: 647-655
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 577 - 591, The early-return branch for unsupported custom tools (when not hasattr(tool_to_call, "function")) builds a ToolCallResult and calls ToolCallingLLM._log_tool_call_result but skips emitting tool metrics; update that branch to invoke the same metric emission used in the normal error/success path (the metrics block at lines 647-655) before returning so tool.call.count/duration/errors are emitted—i.e., after constructing tool_call_result and before return, call the helper that emits tool metrics (using tool_span, tool_call_result, and enable_tool_approval) exactly like the later path does.
♻️ Duplicate comments (5)
docs/reference/opentelemetry.md (1)
130-142:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the DQL fenced block.
Line 130 should specify a language (e.g.,
sqlortext) for consistent rendering and lint compliance.Suggested fix
-``` +```sql # Tool call duration by tool name timeseries avg(holmesgpt.tool.call.duration, default:0), by: {holmesgpt_tool_name}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/opentelemetry.md` around lines 130 - 142, The fenced DQL code block lacks a language identifier causing lint/render issues; update the opening fence for the block containing the four timeseries queries (the block starting with "# Tool call duration by tool name") to include a language identifier such as "sql" (e.g., change ``` to ```sql) so the block becomes ```sql and leaves the inner query text unchanged..github/workflows/helm-publish.yml (1)
16-16:⚠️ Potential issue | 🟠 MajorPublish to the project OCI namespace, not a personal namespace.
Line 16 points to
ghcr.io/henrikrexed, so Line 36 pushes charts to the wrong registry path for the project workflow.Suggested fix
env: - CHART_REPO: ghcr.io/henrikrexed + CHART_REPO: ghcr.io/holmesgpt/chartsAlso applies to: 34-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-publish.yml at line 16, The workflow is pushing Helm charts to the personal OCI namespace currently set by CHART_REPO ("ghcr.io/henrikrexed"); update CHART_REPO to the project's OCI namespace and ensure any push steps that reference CHART_REPO (the chart push/publish steps around where charts are packaged and pushed) are left pointing to that variable so they publish to the project registry instead of a personal account.server.py (2)
433-471:⚠️ Potential issue | 🟠 MajorEnsure non-stream root span always closes on error paths.
trace_span.end()only runs on the success path (Lines 468-470). Ifai.call(...)or metric emission fails, the investigation span remains open.Suggested fix
- try: - # Use provided trace_span or create a root investigation span - trace_span = chat_request.trace_span - if trace_span is None: + created_trace_span = chat_request.trace_span is None + trace_span = chat_request.trace_span + try: + if created_trace_span: trace_span = server_tracer.start_trace( "holmesgpt.investigation", ) trace_span.log(metadata={ "holmesgpt.investigation.question": chat_request.ask[:1024], }) @@ - # End root investigation span if we created it - if chat_request.trace_span is None and trace_span is not None: - trace_span.end() return response finally: + if created_trace_span and trace_span is not None: + trace_span.end() storage.__exit__(None, None, None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 433 - 471, trace_span created via server_tracer.start_trace is only ended on the success path; wrap the work that uses trace_span (the call to ai.call, metric emission using get_metrics(), and response construction) in a try/finally so that if chat_request.trace_span is None and we created trace_span it is always ended in the finally block; specifically, after starting trace_span (server_tracer.start_trace) and setting its log, run ai.call(...) and subsequent otel_metrics logic inside try, then in finally call trace_span.end() only when chat_request.trace_span is None and trace_span is not None, re-raising any caught exception so behavior is unchanged.
319-331:⚠️ Potential issue | 🟠 MajorAdd type hints to
_stream_with_trace_cleanup.This helper is newly introduced without typed parameters/return.
As per coding guidelines "Type hints required in Python files (mypy configuration in pyproject.toml)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 319 - 331, Add explicit type hints to the _stream_with_trace_cleanup function signature: annotate storage as a ContextManager[Any] (or more specific context manager type), stream_generator as a Generator[bytes, None, None] (or Iterator[bytes] if appropriate), req_info as str (or a more specific request-info type), trace_span as opentelemetry.trace.Span (or any object with end()), and the function return type as Generator[bytes, None, None]; also ensure you import the necessary typing names (Generator, ContextManager, Any) and Span from opentelemetry.trace at the top of the file so the annotations resolve.holmes/core/tool_calling_llm.py (1)
851-858:⚠️ Potential issue | 🟠 MajorUse per-iteration token values on each
gen_ai.chatspan.Lines 854-856 log cumulative
stats, so each iteration span reports accumulated totals. Also, Line 857 logs a 1-based iteration index.Suggested fix
llm_span.log(metadata={ "gen_ai.system": "litellm", "gen_ai.request.model": self.llm.model, - "gen_ai.usage.input_tokens": stats.prompt_tokens, - "gen_ai.usage.output_tokens": stats.completion_tokens, - "gen_ai.usage.total_tokens": stats.total_tokens, - "holmesgpt.iteration": i, + "gen_ai.usage.input_tokens": response_stats.prompt_tokens, + "gen_ai.usage.output_tokens": response_stats.completion_tokens, + "gen_ai.usage.total_tokens": response_stats.total_tokens, + "holmesgpt.iteration": i - 1, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 851 - 858, The span currently logs cumulative stats and a 1-based iteration index; change llm_span.log to record per-iteration token deltas and a 0-based iteration index by computing differences from the previous iteration (e.g., prompt_tokens_delta = stats.prompt_tokens - prev_prompt_tokens, completion_tokens_delta = stats.completion_tokens - prev_completion_tokens, total_tokens_delta = stats.total_tokens - prev_total_tokens) and log those deltas instead of the cumulative stats, and log iteration as i-1 (or maintain a separate zero_based_iter variable); update/initialize the prev_* variables before the loop and update them after logging so llm_span.log, stats, and i references reflect per-iteration values.
🧹 Nitpick comments (2)
.github/workflows/helm-publish.yml (1)
5-5: Limit publish triggers to long-lived branches/releases.Including
feat/otel-pr-cleanin Line 5 makes publishing behavior branch-specific and brittle after merge.Suggested fix
- branches: [master, main, feat/otel-pr-clean] + branches: [master, main]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-publish.yml at line 5, Remove the brittle feature branch from the workflow trigger by deleting "feat/otel-pr-clean" from the branches array under the branches key and restrict publishing to long-lived branches/releases instead (e.g., keep "master" and "main" and add a release pattern like "release/*" or "releases/*") so the helm-publish.yml branches trigger only runs for stable branches or release branches.holmes/core/tool_calling_llm.py (1)
886-890: Remove duplicate cancellation checks.Lines 886-887 and 889-890 perform the same check back-to-back.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 886 - 890, The code contains a duplicated cancellation check calling cancel_event.is_set() and raising LLMInterruptedError() twice; inside the function in holmes/core/tool_calling_llm.py simply remove the redundant block so only a single if cancel_event and cancel_event.is_set(): raise LLMInterruptedError() remains (keep the first occurrence and delete the second) to avoid double-raising and duplicate logic while preserving the intended cancellation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/opentelemetry.md`:
- Around line 52-62: The unlabeled fenced code blocks showing the trace tree
starting with "holmesgpt.investigation (root span)" and the architecture diagram
further down must include a language identifier (e.g., use ```text) so they
render correctly; update the opening fences for those ASCII diagram blocks in
docs/reference/opentelemetry.md to use ```text (apply the same change to the
other block around the architecture diagram that spans the later section).
- Around line 66-81: The documentation sections with bold labels (e.g.,
"Investigation span (`holmesgpt.investigation`)", "LLM spans (`gen_ai.chat`)",
and "Tool spans (`holmesgpt.tool.<name>`)" ) have lists immediately following
the bold line; insert a single blank line between each bold label line and its
subsequent list so MkDocs renders them properly (add an empty line after each
bold header before the dash-list items).
In `@holmes/core/otel_tracing.py`:
- Line 164: The method set_attributes currently declares a parameter named type
which shadows the built-in; rename this parameter to span_type in
holmes/core/otel_tracing.py (method set_attributes) and update any references to
that parameter in the method body and callers; also mirror the same rename in
the base class method signature (tracing.py:125, tracing.Tracer.set_attributes)
to keep signatures consistent and avoid Ruff A002 warnings while preserving
existing behavior.
- Line 200: The code computes insecure from the traces `endpoint` and then
reuses it for the metrics exporter, which can misconfigure TLS when
`OTEL_EXPORTER_OTLP_METRICS_ENDPOINT` differs; fix by computing a separate TLS
flag for the metrics endpoint (e.g., `metrics_insecure`) based on
`metrics_endpoint` (or parse both endpoints' schemes via urllib.parse.urlparse
and set boolean insecure flags accordingly) and use that `metrics_insecure` when
constructing the metrics exporter instead of reusing `insecure`; update
references near the exporter creation code and ensure variables `endpoint`,
`metrics_endpoint`, `insecure`, and `metrics_insecure` in otel_tracing.py are
used consistently.
In `@server.py`:
- Around line 451-456: Move the investigation_count metric increment to occur
before the LLM invocation so “started” investigations are counted even if
ai.call(...) fails: compute inv_attrs the same way (using chat_request.model or
config.model or "unknown") by calling get_metrics() and invoking
otel_metrics.investigation_count.add(1, inv_attrs) immediately before the
ai.call(...) (or any branching that chooses stream vs non-stream), and keep
otel_metrics.investigation_duration.record(...) after the call to record elapsed
time on completion; ensure you still guard on otel_metrics being truthy and
preserve the existing inv_attrs variable name.
---
Outside diff comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 577-591: The early-return branch for unsupported custom tools
(when not hasattr(tool_to_call, "function")) builds a ToolCallResult and calls
ToolCallingLLM._log_tool_call_result but skips emitting tool metrics; update
that branch to invoke the same metric emission used in the normal error/success
path (the metrics block at lines 647-655) before returning so
tool.call.count/duration/errors are emitted—i.e., after constructing
tool_call_result and before return, call the helper that emits tool metrics
(using tool_span, tool_call_result, and enable_tool_approval) exactly like the
later path does.
---
Duplicate comments:
In @.github/workflows/helm-publish.yml:
- Line 16: The workflow is pushing Helm charts to the personal OCI namespace
currently set by CHART_REPO ("ghcr.io/henrikrexed"); update CHART_REPO to the
project's OCI namespace and ensure any push steps that reference CHART_REPO (the
chart push/publish steps around where charts are packaged and pushed) are left
pointing to that variable so they publish to the project registry instead of a
personal account.
In `@docs/reference/opentelemetry.md`:
- Around line 130-142: The fenced DQL code block lacks a language identifier
causing lint/render issues; update the opening fence for the block containing
the four timeseries queries (the block starting with "# Tool call duration by
tool name") to include a language identifier such as "sql" (e.g., change ``` to
```sql) so the block becomes ```sql and leaves the inner query text unchanged.
In `@holmes/core/tool_calling_llm.py`:
- Around line 851-858: The span currently logs cumulative stats and a 1-based
iteration index; change llm_span.log to record per-iteration token deltas and a
0-based iteration index by computing differences from the previous iteration
(e.g., prompt_tokens_delta = stats.prompt_tokens - prev_prompt_tokens,
completion_tokens_delta = stats.completion_tokens - prev_completion_tokens,
total_tokens_delta = stats.total_tokens - prev_total_tokens) and log those
deltas instead of the cumulative stats, and log iteration as i-1 (or maintain a
separate zero_based_iter variable); update/initialize the prev_* variables
before the loop and update them after logging so llm_span.log, stats, and i
references reflect per-iteration values.
In `@server.py`:
- Around line 433-471: trace_span created via server_tracer.start_trace is only
ended on the success path; wrap the work that uses trace_span (the call to
ai.call, metric emission using get_metrics(), and response construction) in a
try/finally so that if chat_request.trace_span is None and we created trace_span
it is always ended in the finally block; specifically, after starting trace_span
(server_tracer.start_trace) and setting its log, run ai.call(...) and subsequent
otel_metrics logic inside try, then in finally call trace_span.end() only when
chat_request.trace_span is None and trace_span is not None, re-raising any
caught exception so behavior is unchanged.
- Around line 319-331: Add explicit type hints to the _stream_with_trace_cleanup
function signature: annotate storage as a ContextManager[Any] (or more specific
context manager type), stream_generator as a Generator[bytes, None, None] (or
Iterator[bytes] if appropriate), req_info as str (or a more specific
request-info type), trace_span as opentelemetry.trace.Span (or any object with
end()), and the function return type as Generator[bytes, None, None]; also
ensure you import the necessary typing names (Generator, ContextManager, Any)
and Span from opentelemetry.trace at the top of the file so the annotations
resolve.
---
Nitpick comments:
In @.github/workflows/helm-publish.yml:
- Line 5: Remove the brittle feature branch from the workflow trigger by
deleting "feat/otel-pr-clean" from the branches array under the branches key and
restrict publishing to long-lived branches/releases instead (e.g., keep "master"
and "main" and add a release pattern like "release/*" or "releases/*") so the
helm-publish.yml branches trigger only runs for stable branches or release
branches.
In `@holmes/core/tool_calling_llm.py`:
- Around line 886-890: The code contains a duplicated cancellation check calling
cancel_event.is_set() and raising LLMInterruptedError() twice; inside the
function in holmes/core/tool_calling_llm.py simply remove the redundant block so
only a single if cancel_event and cancel_event.is_set(): raise
LLMInterruptedError() remains (keep the first occurrence and delete the second)
to avoid double-raising and duplicate logic while preserving the intended
cancellation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 659d09be-a555-4db2-833c-7b15eeb2fc61
📥 Commits
Reviewing files that changed from the base of the PR and between 3043d378c9ca0ad26e5138bf21ca14b63ff1ab87 and 52f00893aeb31ea497cff6d4f5c56aafa37ee8e1.
📒 Files selected for processing (11)
.github/workflows/helm-publish.ymlDockerfiledocs/reference/opentelemetry.mdholmes/core/otel_tracing.pyholmes/core/tool_calling_llm.pyholmes/core/tracing.pyholmes/main.pymkdocs.ymlpyproject.tomlserver.pytests/test_otel_tracing.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- holmes/main.py
- Dockerfile
- tests/test_otel_tracing.py
- holmes/core/tracing.py
henrikrexed
force-pushed
the
feat/otel-pr-clean
branch
from
March 17, 2026 16:55
52f0089 to
6cd872f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (11)
docs/reference/opentelemetry.md (4)
198-223:⚠️ Potential issue | 🟡 MinorAdd language identifier to architecture diagram.
The ASCII architecture diagram should use a language identifier (e.g.,
text).📝 Proposed fix
-``` +```text ┌──────────────────────────────────────────────────────┐ │ HolmesGPT │🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/opentelemetry.md` around lines 198 - 223, The fenced ASCII diagram block lacks a language identifier; update the diagram's opening fence to include a language tag (for example use ```text) and ensure the closing triple backticks remain, locating the block by the unique diagram content (e.g., lines containing "HolmesGPT", "OTel Collector", "W3C traceparent") so the diagram is rendered with the proper language identifier.
52-62:⚠️ Potential issue | 🟡 MinorAdd language identifier to fenced code blocks.
The trace tree diagram should use a language identifier (e.g.,
text) for consistent formatting.📝 Proposed fix
-``` +```text holmesgpt.investigation (root span) ├── gen_ai.chat (LLM iteration 0 — includes token counts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/opentelemetry.md` around lines 52 - 62, The fenced code block showing the trace tree lacks a language identifier; update the block to include a language tag (e.g., "text") so it renders consistently (modify the fenced block that contains holmesgpt.investigation (root span) and the gen_ai.chat / holmesgpt.tool.<name> lines to start with ```text instead of ```) ensuring all similar diagrams in this file follow the same pattern.
66-81:⚠️ Potential issue | 🟡 MinorAdd blank lines between bold labels and their lists.
Per coding guidelines, documentation must have a blank line between bold text/headers and lists for proper MkDocs rendering. The bold section labels on lines 66, 70, and 78 are immediately followed by lists.
📝 Proposed fix
**Investigation span** (`holmesgpt.investigation`): + - `holmesgpt.investigation.question` — the user's question - `holmesgpt.investigation.stream` — whether streaming was used **LLM spans** (`gen_ai.chat`): + - `gen_ai.system` — LLM provider (`litellm`) - `gen_ai.request.model` — model name - `gen_ai.usage.input_tokens` — prompt tokens - `gen_ai.usage.output_tokens` — completion tokens - `gen_ai.usage.total_tokens` — total tokens - `holmesgpt.iteration` — iteration number (0-based) **Tool spans** (`holmesgpt.tool.<name>`): + - `holmesgpt.tool.name` — tool name - `holmesgpt.tool.status` — result status (`success` or `error`)As per coding guidelines: "In documentation, always add a blank line between a header/bold text and a list to ensure proper MkDocs rendering".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/opentelemetry.md` around lines 66 - 81, Add a blank line between each bold label and its following list to satisfy MkDocs rendering: locate the bold sections "Investigation span" (holmesgpt.investigation), "LLM spans" (gen_ai.chat) and "Tool spans" (holmesgpt.tool.<name>) and insert an empty line after each bolded line so the bullet lists under holmesgpt.investigation, gen_ai.chat, and holmesgpt.tool.<name> are separated from their headings.
130-142:⚠️ Potential issue | 🟡 MinorAdd language identifier to DQL code block.
The Dynatrace DQL queries block should have a language identifier for consistent formatting.
📝 Proposed fix
-``` +```sql # Tool call duration by tool name timeseries avg(holmesgpt.tool.call.duration, default:0), by: {holmesgpt_tool_name}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/opentelemetry.md` around lines 130 - 142, Update the fenced code block that contains the Dynatrace DQL queries so it includes a language identifier (e.g., change the opening ``` to ```sql or ```dql) to enable consistent syntax highlighting; locate the block containing the queries such as "timeseries avg(holmesgpt.tool.call.duration, default:0), by: {holmesgpt_tool_name}" and the other lines ("timeseries sum(gen_ai.client.token.usage...)", "timeseries sum(holmesgpt.investigation.count...)", "timeseries percentile(holmesgpt.tool.call.duration, 95)...") and replace the opening fence accordingly.holmes/core/tool_calling_llm.py (1)
850-858:⚠️ Potential issue | 🟠 MajorUse per-iteration token counts instead of cumulative stats.
The span attributes at lines 854-856 use
stats.prompt_tokens(cumulative) while the metrics at lines 843-846 correctly useresponse_stats(per-iteration). The span should also use per-iteration values for consistency.💡 Proposed fix
llm_span.log(metadata={ "gen_ai.system": "litellm", "gen_ai.request.model": self.llm.model, - "gen_ai.usage.input_tokens": stats.prompt_tokens, - "gen_ai.usage.output_tokens": stats.completion_tokens, - "gen_ai.usage.total_tokens": stats.total_tokens, + "gen_ai.usage.input_tokens": response_stats.prompt_tokens, + "gen_ai.usage.output_tokens": response_stats.completion_tokens, + "gen_ai.usage.total_tokens": response_stats.total_tokens, "holmesgpt.iteration": i, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 850 - 858, The llm_span.log call is using cumulative stats (stats.prompt_tokens, stats.completion_tokens, stats.total_tokens) but should use the per-iteration values already computed as response_stats; update the metadata keys in the llm_span.log call (in the llm_span.log block) to reference response_stats.prompt_tokens, response_stats.completion_tokens, and response_stats.total_tokens (keeping gen_ai.system, gen_ai.request.model and holmesgpt.iteration unchanged) so the span attributes match the per-iteration metrics recorded earlier..github/workflows/helm-publish.yml (1)
1-36:⚠️ Potential issue | 🟠 MajorThis workflow duplicates existing Helm publishing and targets a personal registry.
The existing workflow in
.github/workflows/build-docker-images.yamlalready publishes Helm charts toghcr.io/holmesgpt/chartson release. This new workflow:
- Targets a personal namespace (
ghcr.io/henrikrexed) instead of the project registry- Triggers on the feature branch
feat/otel-pr-clean, which shouldn't be in production config- Creates a duplicate publishing path that can cause confusion
Consider removing this workflow entirely, as the existing
build-docker-images.yamlalready handles Helm chart publishing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-publish.yml around lines 1 - 36, This workflow duplicates existing Helm publishing; remove or disable it: delete the GitHub Actions workflow that defines the helm-publish job (the workflow with name "Helm — Package & Push OCI" which sets env CHART_REPO to ghcr.io/henrikrexed and triggers on branches [master, main, feat/otel-pr-clean]) or, if you intend to keep it, change CHART_REPO to the project registry (ghcr.io/holmesgpt/charts), remove the feature branch trigger (feat/otel-pr-clean) from the push branches list, and ensure it does not overlap release triggers already handled by build-docker-images.yaml so only one workflow publishes Helm charts.holmes/core/otel_tracing.py (2)
200-220:⚠️ Potential issue | 🟠 MajorDerive the metrics TLS mode from
metrics_endpoint, notendpoint.
OTEL_EXPORTER_OTLP_METRICS_ENDPOINTcan use a different scheme than the trace exporter. Reusing the trace endpoint'sinsecureflag will misconfigure the metrics exporter whenever the two endpoints differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/otel_tracing.py` around lines 200 - 220, The metrics exporter currently reuses the TLS boolean computed for traces (variable insecure derived from endpoint), which is wrong when OTEL_EXPORTER_OTLP_METRICS_ENDPOINT differs; compute a separate flag (e.g., insecure_metrics) from metrics_endpoint (check its scheme like metrics_endpoint.startswith("https://")) and pass that to OTLPMetricExporter instead of the trace-derived insecure so OTLPMetricExporter(endpoint=metrics_endpoint, insecure=insecure_metrics, headers=...) is correctly configured.
164-164:⚠️ Potential issue | 🟡 MinorRename
typeto avoid the Ruff A002 violation.This parameter still shadows Python's builtin
type, and Ruff already reports it. Please rename it here and in the matching base signature.As per coding guidelines "Use Ruff for formatting and linting in Python files (configured in pyproject.toml)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/otel_tracing.py` at line 164, The parameter name type in the set_attributes method shadows Python's builtin and triggers Ruff A002; rename it (e.g., span_type or attr_type) in the def set_attributes(self, name: Optional[str] = None, type: Optional[str] = None, ...) signature and update the corresponding base method/signature it overrides (the matching base/class method) and all internal references and call sites to use the new name so the change is consistent across the class hierarchy.server.py (3)
433-471:⚠️ Potential issue | 🟠 MajorEnd owned investigation spans in a
finally.When this request creates the root span, it is only ended on the success path. Exceptions from
ai.call(...), metric emission, orChatResponse(...)leave the span open.
451-456:⚠️ Potential issue | 🟠 MajorCount investigations before invoking
ai.call(...).The instrument is described as counting started investigations, but here it increments only after the LLM call succeeds. Failed investigations disappear from the metric.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 451 - 456, The investigation count is incremented only after ai.call(...) succeeds, so move the invocation of otel_metrics.investigation_count.add(1, inv_attrs) to just before the AI call to count started investigations (use get_metrics() -> otel_metrics and the same inv_attrs containing chat_request.model or config.model or "unknown"); ensure _inv_start is set before this increment so investigation_duration.record(time.time() - _inv_start, inv_attrs) can still be recorded afterwards (and keep the duration.record call after the AI call or in finally to capture failures).
319-331:⚠️ Potential issue | 🟡 MinorAdd type hints to
_stream_with_trace_cleanup.This new helper is still untyped. Please annotate the storage context manager, stream iterator/generator, span argument, and return type so mypy can validate this cleanup path.
As per coding guidelines "Type hints required in Python files (mypy configuration in pyproject.toml)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 319 - 331, Annotate _stream_with_trace_cleanup with precise type hints: set req_info to str (or a specific RequestInfo type if available), storage to ContextManager[Iterator[bytes]] (or ContextManager[Generator[bytes, None, None]]), stream_generator to Iterator[bytes] or Generator[bytes, None, None], trace_span to opentelemetry.trace.Span (or a protocol/Any with an end() -> None method), and the function return type to Iterator[bytes] / Generator[bytes, None, None]; import the needed types from typing (ContextManager, Iterator, Generator, Any) and opentelemetry.trace for Span to satisfy mypy.
🧹 Nitpick comments (2)
tests/test_otel_tracing.py (1)
26-28: Accessing private OTel internals for test isolation is fragile but necessary.The comment explains the rationale well. Consider adding a note about potential breakage if OTel SDK changes internal structure.
📝 Suggested documentation improvement
# Reset the global provider guard so we can set a fresh provider per test + # NOTE: This accesses OTel SDK internals and may break with SDK updates. + # Monitor opentelemetry-sdk releases if tests start failing. trace._TRACER_PROVIDER_SET_ONCE._done = False # type: ignore[attr-defined]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_otel_tracing.py` around lines 26 - 28, Add a short explanatory note near the test code that resets OTel internals (the trace._TRACER_PROVIDER_SET_ONCE._done assignment before calling trace.set_tracer_provider(provider)) stating this touches private OTel SDK internals, is fragile, and may break if OTel changes its internal structure; keep the note brief and mention that the test relies on this hack for isolation and should be revisited if OTel SDK is upgraded.holmes/core/otel_tracing.py (1)
269-276: Move the optionalHTTPXClientInstrumentorimport to module scope.The lazy import preserves the optional behavior, but it still violates the repo's Python import rule. Keep the availability check, just express it with a separate top-level optional import flag instead of importing inside
__init__.As per coding guidelines "Always place Python imports at the top of the file, not inside functions or methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/otel_tracing.py` around lines 269 - 276, The HTTPXClientInstrumentor import should be moved to module scope and guarded by an ImportError flag: at top of the module attempt "from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor" in a try/except and set a boolean like "HTTPX_INSTRUMENTOR_AVAILABLE = True/False" (or set HTTPXClientInstrumentor = None) depending on success; then in the existing instrumentation block replace the in-function import with a conditional that checks that flag (or non-None symbol) and only calls HTTPXClientInstrumentor().instrument() when available, otherwise log the same warning—this preserves optional behavior while keeping imports at module scope and uses the existing symbol HTTPXClientInstrumentor in the instrumentation code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 886-890: Remove the duplicated cancel_event check that raises
LLMInterruptedError: there are two consecutive blocks checking "if cancel_event
and cancel_event.is_set(): raise LLMInterruptedError()"; keep only one such
check (retain the original existing check and delete the duplicate added in the
PR) in the function in holmes.core.tool_calling_llm (the duplicate references
the same cancel_event and LLMInterruptedError symbols).
In `@server.py`:
- Around line 411-429: Streaming branch currently only increments
investigation_count; move or add recording of investigation_duration and
investigation_iterations into the stream finalization path (the same wrapper
that performs cleanup) so streaming requests emit the same histograms as
non-streaming. Specifically, modify the cleanup wrapper function
(_stream_with_trace_cleanup or the generator returned by stream_chat_formatter)
to compute the duration and iteration count when the stream completes or errors
and call otel_metrics.investigation_duration.record(...) and
otel_metrics.investigation_iterations.record(...) with the same inv_attrs used
for investigation_count; ensure otel_metrics is fetched (get_metrics()) and that
inv_attrs uses chat_request.model or config.model fallback so the metrics match
the non-stream branch.
- Around line 405-410: The streaming path always creates and closes a fresh root
span; instead check for an existing chat_request.trace_span and reuse it to
preserve trace continuity: if chat_request.trace_span is present assign
trace_span = chat_request.trace_span and do not mark it for closing, otherwise
create a new root via server_tracer.start_trace("holmesgpt.investigation"), log
the metadata on trace_span as before, track a small created_root_span boolean
(or similar) to indicate ownership, and only call trace_span.end() (or close it)
when created_root_span is True; apply the same change to the other
streaming-location mentioned (around the second occurrence).
- Around line 407-410: The trace_span.log call is exporting raw user input
(chat_request.ask[:1024]) into telemetry which risks privacy and
high-cardinality; change trace_span.log usage to avoid sending raw prompts by
replacing chat_request.ask[:1024] with a privacy-preserving artifact such as a
deterministic hash (e.g., sha256 of chat_request.ask), the input length, and an
optional redacted/normalized summary (or a tokenized/first-n words stub), and
only include full text under an explicit debug opt-in flag; update both
occurrences around trace_span.log and any other uses referencing
chat_request.ask to use the hash/length/summary pattern or respect the debug
opt-in.
---
Duplicate comments:
In @.github/workflows/helm-publish.yml:
- Around line 1-36: This workflow duplicates existing Helm publishing; remove or
disable it: delete the GitHub Actions workflow that defines the helm-publish job
(the workflow with name "Helm — Package & Push OCI" which sets env CHART_REPO to
ghcr.io/henrikrexed and triggers on branches [master, main, feat/otel-pr-clean])
or, if you intend to keep it, change CHART_REPO to the project registry
(ghcr.io/holmesgpt/charts), remove the feature branch trigger
(feat/otel-pr-clean) from the push branches list, and ensure it does not overlap
release triggers already handled by build-docker-images.yaml so only one
workflow publishes Helm charts.
In `@docs/reference/opentelemetry.md`:
- Around line 198-223: The fenced ASCII diagram block lacks a language
identifier; update the diagram's opening fence to include a language tag (for
example use ```text) and ensure the closing triple backticks remain, locating
the block by the unique diagram content (e.g., lines containing "HolmesGPT",
"OTel Collector", "W3C traceparent") so the diagram is rendered with the proper
language identifier.
- Around line 52-62: The fenced code block showing the trace tree lacks a
language identifier; update the block to include a language tag (e.g., "text")
so it renders consistently (modify the fenced block that contains
holmesgpt.investigation (root span) and the gen_ai.chat / holmesgpt.tool.<name>
lines to start with ```text instead of ```) ensuring all similar diagrams in
this file follow the same pattern.
- Around line 66-81: Add a blank line between each bold label and its following
list to satisfy MkDocs rendering: locate the bold sections "Investigation span"
(holmesgpt.investigation), "LLM spans" (gen_ai.chat) and "Tool spans"
(holmesgpt.tool.<name>) and insert an empty line after each bolded line so the
bullet lists under holmesgpt.investigation, gen_ai.chat, and
holmesgpt.tool.<name> are separated from their headings.
- Around line 130-142: Update the fenced code block that contains the Dynatrace
DQL queries so it includes a language identifier (e.g., change the opening ```
to ```sql or ```dql) to enable consistent syntax highlighting; locate the block
containing the queries such as "timeseries avg(holmesgpt.tool.call.duration,
default:0), by: {holmesgpt_tool_name}" and the other lines ("timeseries
sum(gen_ai.client.token.usage...)", "timeseries
sum(holmesgpt.investigation.count...)", "timeseries
percentile(holmesgpt.tool.call.duration, 95)...") and replace the opening fence
accordingly.
In `@holmes/core/otel_tracing.py`:
- Around line 200-220: The metrics exporter currently reuses the TLS boolean
computed for traces (variable insecure derived from endpoint), which is wrong
when OTEL_EXPORTER_OTLP_METRICS_ENDPOINT differs; compute a separate flag (e.g.,
insecure_metrics) from metrics_endpoint (check its scheme like
metrics_endpoint.startswith("https://")) and pass that to OTLPMetricExporter
instead of the trace-derived insecure so
OTLPMetricExporter(endpoint=metrics_endpoint, insecure=insecure_metrics,
headers=...) is correctly configured.
- Line 164: The parameter name type in the set_attributes method shadows
Python's builtin and triggers Ruff A002; rename it (e.g., span_type or
attr_type) in the def set_attributes(self, name: Optional[str] = None, type:
Optional[str] = None, ...) signature and update the corresponding base
method/signature it overrides (the matching base/class method) and all internal
references and call sites to use the new name so the change is consistent across
the class hierarchy.
In `@holmes/core/tool_calling_llm.py`:
- Around line 850-858: The llm_span.log call is using cumulative stats
(stats.prompt_tokens, stats.completion_tokens, stats.total_tokens) but should
use the per-iteration values already computed as response_stats; update the
metadata keys in the llm_span.log call (in the llm_span.log block) to reference
response_stats.prompt_tokens, response_stats.completion_tokens, and
response_stats.total_tokens (keeping gen_ai.system, gen_ai.request.model and
holmesgpt.iteration unchanged) so the span attributes match the per-iteration
metrics recorded earlier.
In `@server.py`:
- Around line 451-456: The investigation count is incremented only after
ai.call(...) succeeds, so move the invocation of
otel_metrics.investigation_count.add(1, inv_attrs) to just before the AI call to
count started investigations (use get_metrics() -> otel_metrics and the same
inv_attrs containing chat_request.model or config.model or "unknown"); ensure
_inv_start is set before this increment so
investigation_duration.record(time.time() - _inv_start, inv_attrs) can still be
recorded afterwards (and keep the duration.record call after the AI call or in
finally to capture failures).
- Around line 319-331: Annotate _stream_with_trace_cleanup with precise type
hints: set req_info to str (or a specific RequestInfo type if available),
storage to ContextManager[Iterator[bytes]] (or ContextManager[Generator[bytes,
None, None]]), stream_generator to Iterator[bytes] or Generator[bytes, None,
None], trace_span to opentelemetry.trace.Span (or a protocol/Any with an end()
-> None method), and the function return type to Iterator[bytes] /
Generator[bytes, None, None]; import the needed types from typing
(ContextManager, Iterator, Generator, Any) and opentelemetry.trace for Span to
satisfy mypy.
---
Nitpick comments:
In `@holmes/core/otel_tracing.py`:
- Around line 269-276: The HTTPXClientInstrumentor import should be moved to
module scope and guarded by an ImportError flag: at top of the module attempt
"from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor" in a
try/except and set a boolean like "HTTPX_INSTRUMENTOR_AVAILABLE = True/False"
(or set HTTPXClientInstrumentor = None) depending on success; then in the
existing instrumentation block replace the in-function import with a conditional
that checks that flag (or non-None symbol) and only calls
HTTPXClientInstrumentor().instrument() when available, otherwise log the same
warning—this preserves optional behavior while keeping imports at module scope
and uses the existing symbol HTTPXClientInstrumentor in the instrumentation
code.
In `@tests/test_otel_tracing.py`:
- Around line 26-28: Add a short explanatory note near the test code that resets
OTel internals (the trace._TRACER_PROVIDER_SET_ONCE._done assignment before
calling trace.set_tracer_provider(provider)) stating this touches private OTel
SDK internals, is fragile, and may break if OTel changes its internal structure;
keep the note brief and mention that the test relies on this hack for isolation
and should be revisited if OTel SDK is upgraded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d6c8bfc-54e5-4b1b-b49f-fb8c67b60d89
📥 Commits
Reviewing files that changed from the base of the PR and between 52f00893aeb31ea497cff6d4f5c56aafa37ee8e1 and 6cd872f.
📒 Files selected for processing (11)
.github/workflows/helm-publish.ymlDockerfiledocs/reference/opentelemetry.mdholmes/core/otel_tracing.pyholmes/core/tool_calling_llm.pyholmes/core/tracing.pyholmes/main.pymkdocs.ymlpyproject.tomlserver.pytests/test_otel_tracing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
Contributor
Author
|
The DCO check flags commits from upstream/master that were included via merge. Those commits are authored by other contributors and already merged into the main branch , they are not part of this PR's changes. |
Collaborator
|
@henrikrexed thanks, I'll fix the DCO manually. Note that there are actually three PRs for adding OpenTelemetry:
@Avi-Robusta from our team is reviewing everything this week. We'll make sure everyone's requirements get merged without adding duplicate code. That probably means merging one as a base and then some modifications based on the others. |
This was referenced Mar 22, 2026
Avi-Robusta
reviewed
Mar 22, 2026
Avi-Robusta
reviewed
Mar 22, 2026
Avi-Robusta
reviewed
Mar 22, 2026
Avi-Robusta
reviewed
Mar 22, 2026
Avi-Robusta
reviewed
Mar 22, 2026
Avi-Robusta
reviewed
Mar 22, 2026
Avi-Robusta
reviewed
Mar 22, 2026
Avi-Robusta
reviewed
Mar 22, 2026
Avi-Robusta
reviewed
Mar 22, 2026
Avi-Robusta
requested changes
Mar 22, 2026
Avi-Robusta
left a comment
Avi-Robusta
left a comment
Collaborator
There was a problem hiding this comment.
Looks good, some blocking changes
But great work!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
server.py (4)
406-409:⚠️ Potential issue | 🟠 MajorDo not export raw prompts into span attributes.
chat_request.ask[:1024]pushes end-user prompt text into the tracing backend and creates very high-cardinality telemetry. Prefer a hash/length or an explicit debug opt-in instead.Also applies to: 438-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 406 - 409, The trace_span.log call currently writes raw user prompt text via chat_request.ask[:1024], creating high-cardinality telemetry; change trace_span.log to avoid exporting raw prompt text by instead recording a non-sensitive identifier such as a hash (e.g., SHA256 of chat_request.ask) and/or the prompt length, or gate full-text inclusion behind an explicit debug opt-in flag; update both occurrences (the trace_span.log that references chat_request.ask[:1024] and the similar block at the later lines) to log only the hash/length or opt-in marker rather than the raw prompt.
403-409:⚠️ Potential issue | 🟠 MajorReuse an incoming
chat_request.trace_spanfor streaming.This branch always starts and later closes a fresh investigation span. If the caller already supplied
chat_request.trace_span, streaming loses trace continuity while the non-stream path preserves it. Reuse the provided span and only end it when this handler created it.Also applies to: 422-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 403 - 409, The streaming branch currently always creates a new investigation span via server_tracer.start_trace("holmesgpt.investigation") which breaks trace continuity when chat_request.trace_span is supplied; change the logic in the streaming branch to reuse chat_request.trace_span if present (assign trace_span = chat_request.trace_span), otherwise start a new span and mark that this handler created it (e.g., created_internal_span = True); keep the logging of metadata on the chosen trace_span and ensure you only call trace_span.finish()/end when created_internal_span is True so externally-provided spans are not closed here (apply same fix to the other identical block around the later lines noted).
432-469:⚠️ Potential issue | 🟠 MajorEnd the root span on every non-stream exit path.
If
ai.call(), metric emission, orChatResponse(...)raises afterstart_trace(), the span is never ended becausetrace_span.end()sits on the success path only. Track span ownership and move theend()into afinally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 432 - 469, The current logic starts a root span via server_tracer.start_trace() into trace_span when chat_request.trace_span is None but only calls trace_span.end() on the success path; to fix, track ownership (e.g., a boolean created_root_span when you call server_tracer.start_trace()) and ensure trace_span.end() is invoked in a finally block that runs on all exit paths, wrapping the ai.call(...), TracingFactory.get_metrics use, and ChatResponse(...) construction so any exceptions still trigger ending the span; reference trace_span, chat_request.trace_span, server_tracer.start_trace, ai.call, TracingFactory.get_metrics, and ChatResponse to locate where to add the ownership flag and finally/end logic.
318-330:⚠️ Potential issue | 🟠 MajorRecord streaming investigation duration and iteration histograms.
Streaming requests increment
investigation_count, but neither this cleanup wrapper nor the streaming branch recordsinvestigation_durationorinvestigation_iterations. Those histograms will miss all streamed investigations.Also applies to: 410-413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 318 - 330, The streaming wrapper _stream_with_trace_cleanup currently yields directly from stream_generator and never records investigation_duration or investigation_iterations; change it to wrap stream_generator with a small generator that records start = time.monotonic(), increments a local iterations counter on each yield (e.g., inside a for/while that yields items from stream_generator), and in the finally block call investigation_duration.observe(time.monotonic() - start) and investigation_iterations.observe(iterations) before trace_span.end() and storage.__exit__(...). Ensure you reference the same symbols (_stream_with_trace_cleanup, stream_generator, trace_span, storage) so the iteration counting occurs during streaming and metrics are updated even on errors/early termination.holmes/core/otel_tracing.py (2)
208-213:⚠️ Potential issue | 🟠 MajorDerive metric exporter TLS mode from
metrics_endpoint.
insecureis computed from the trace endpoint and then reused for the metric exporter. IfOTEL_EXPORTER_OTLP_METRICS_ENDPOINTpoints to a different scheme, metrics will be initialized with the wrong TLS setting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/otel_tracing.py` around lines 208 - 213, The code reuses the trace-derived TLS flag `insecure` when creating `OTLPMetricExporter`, but TLS should be derived from `metrics_endpoint` instead; update the logic so you compute a separate boolean (e.g., `metrics_insecure`) from `metrics_endpoint`'s scheme (use `urlparse(metrics_endpoint).scheme` or check for "https://" prefix) and pass that to `OTLPMetricExporter` instead of the trace `insecure`, ensuring `metrics_endpoint`, `metrics_insecure`, and `OTLPMetricExporter` are used for the metric exporter initialization.
158-158:⚠️ Potential issue | 🟠 MajorRename
typeto avoid Ruff A002.This parameter shadows Python's builtin
type, and Ruff already reports it as A002. Rename it tospan_typehere and in the matching interface inholmes/core/tracing.pyso the signatures stay aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/otel_tracing.py` at line 158, Rename the parameter named type in the set_attributes method to span_type to avoid shadowing the built-in and Ruff A002: update the signature of set_attributes (in otel_tracing.py) from def set_attributes(self, name: Optional[str] = None, type: Optional[str] = None, ...) to use span_type and update the matching declaration/signature in tracing.py so they stay aligned; then update all internal usages and any callers, variable references, and type hints that passed or referenced the old parameter name to use span_type instead.holmes/core/tool_calling_llm.py (1)
848-856:⚠️ Potential issue | 🟠 MajorLog per-iteration token usage on each
gen_ai.chatspan.These attributes still read from cumulative
stats, so the second and later child spans report accumulated totals instead of that iteration's usage. Useresponse_statsor deltas here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tool_calling_llm.py` around lines 848 - 856, The child-span logging is using cumulative stats (stats.prompt_tokens, stats.completion_tokens, stats.total_tokens) so later gen_ai.chat spans show accumulated totals; change the llm_span.log call to use per-response stats (response_stats.prompt_tokens, response_stats.completion_tokens, response_stats.total_tokens) or compute the delta from the previous cumulative values before logging. Update the llm_span.log metadata keys ("gen_ai.usage.input_tokens", "gen_ai.usage.output_tokens", "gen_ai.usage.total_tokens") to read from response_stats (or the computed deltas) and keep "holmesgpt.iteration": i unchanged so each iteration records only that iteration's token usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 647-653: The metrics block currently only increments
tool_call_errors for status.value == "error"; update it to also count
approval-blocked executions when approvals are disabled by checking
tool_call_result.result.status.value == "APPROVAL_REQUIRED" (or equivalent enum)
and your approvals feature flag (e.g., approvals_enabled /
is_approvals_enabled()) is false, then call otel_metrics.tool_call_errors.add(1,
tool_attrs); keep existing otel_metrics, tool_attrs, tool_call_errors and
tool_call_duration logic and only add the extra conditional branch that treats
APPROVAL_REQUIRED as an error when approvals are turned off.
- Around line 570-573: The span name prefix "holmesgpt.tool." is being
overwritten later; modify the call site or _log_tool_call_result so it no longer
sets the span's name attribute to the bare tool name. Specifically, stop calling
tool_span.set_attributes(name=tool_call_result.tool_name) inside
_log_tool_call_result (or change it to set a different attribute key such as
"tool_name") so the span created with
trace_span.start_span(name=f"holmesgpt.tool.{tool_name_for_span}", type="tool")
retains its prefixed name for export and querying.
---
Duplicate comments:
In `@holmes/core/otel_tracing.py`:
- Around line 208-213: The code reuses the trace-derived TLS flag `insecure`
when creating `OTLPMetricExporter`, but TLS should be derived from
`metrics_endpoint` instead; update the logic so you compute a separate boolean
(e.g., `metrics_insecure`) from `metrics_endpoint`'s scheme (use
`urlparse(metrics_endpoint).scheme` or check for "https://" prefix) and pass
that to `OTLPMetricExporter` instead of the trace `insecure`, ensuring
`metrics_endpoint`, `metrics_insecure`, and `OTLPMetricExporter` are used for
the metric exporter initialization.
- Line 158: Rename the parameter named type in the set_attributes method to
span_type to avoid shadowing the built-in and Ruff A002: update the signature of
set_attributes (in otel_tracing.py) from def set_attributes(self, name:
Optional[str] = None, type: Optional[str] = None, ...) to use span_type and
update the matching declaration/signature in tracing.py so they stay aligned;
then update all internal usages and any callers, variable references, and type
hints that passed or referenced the old parameter name to use span_type instead.
In `@holmes/core/tool_calling_llm.py`:
- Around line 848-856: The child-span logging is using cumulative stats
(stats.prompt_tokens, stats.completion_tokens, stats.total_tokens) so later
gen_ai.chat spans show accumulated totals; change the llm_span.log call to use
per-response stats (response_stats.prompt_tokens,
response_stats.completion_tokens, response_stats.total_tokens) or compute the
delta from the previous cumulative values before logging. Update the
llm_span.log metadata keys ("gen_ai.usage.input_tokens",
"gen_ai.usage.output_tokens", "gen_ai.usage.total_tokens") to read from
response_stats (or the computed deltas) and keep "holmesgpt.iteration": i
unchanged so each iteration records only that iteration's token usage.
In `@server.py`:
- Around line 406-409: The trace_span.log call currently writes raw user prompt
text via chat_request.ask[:1024], creating high-cardinality telemetry; change
trace_span.log to avoid exporting raw prompt text by instead recording a
non-sensitive identifier such as a hash (e.g., SHA256 of chat_request.ask)
and/or the prompt length, or gate full-text inclusion behind an explicit debug
opt-in flag; update both occurrences (the trace_span.log that references
chat_request.ask[:1024] and the similar block at the later lines) to log only
the hash/length or opt-in marker rather than the raw prompt.
- Around line 403-409: The streaming branch currently always creates a new
investigation span via server_tracer.start_trace("holmesgpt.investigation")
which breaks trace continuity when chat_request.trace_span is supplied; change
the logic in the streaming branch to reuse chat_request.trace_span if present
(assign trace_span = chat_request.trace_span), otherwise start a new span and
mark that this handler created it (e.g., created_internal_span = True); keep the
logging of metadata on the chosen trace_span and ensure you only call
trace_span.finish()/end when created_internal_span is True so
externally-provided spans are not closed here (apply same fix to the other
identical block around the later lines noted).
- Around line 432-469: The current logic starts a root span via
server_tracer.start_trace() into trace_span when chat_request.trace_span is None
but only calls trace_span.end() on the success path; to fix, track ownership
(e.g., a boolean created_root_span when you call server_tracer.start_trace())
and ensure trace_span.end() is invoked in a finally block that runs on all exit
paths, wrapping the ai.call(...), TracingFactory.get_metrics use, and
ChatResponse(...) construction so any exceptions still trigger ending the span;
reference trace_span, chat_request.trace_span, server_tracer.start_trace,
ai.call, TracingFactory.get_metrics, and ChatResponse to locate where to add the
ownership flag and finally/end logic.
- Around line 318-330: The streaming wrapper _stream_with_trace_cleanup
currently yields directly from stream_generator and never records
investigation_duration or investigation_iterations; change it to wrap
stream_generator with a small generator that records start = time.monotonic(),
increments a local iterations counter on each yield (e.g., inside a for/while
that yields items from stream_generator), and in the finally block call
investigation_duration.observe(time.monotonic() - start) and
investigation_iterations.observe(iterations) before trace_span.end() and
storage.__exit__(...). Ensure you reference the same symbols
(_stream_with_trace_cleanup, stream_generator, trace_span, storage) so the
iteration counting occurs during streaming and metrics are updated even on
errors/early termination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df528068-53e9-46fe-8246-3778051549c8
📥 Commits
Reviewing files that changed from the base of the PR and between 6cd872f and cb5da956ebace505013ae519ec744c3d8cd5fb05.
📒 Files selected for processing (6)
docs/reference/.nav.ymlholmes/core/otel_tracing.pyholmes/core/tool_calling_llm.pyholmes/core/tracing.pypyproject.tomlserver.py
✅ Files skipped from review due to trivial changes (1)
- docs/reference/.nav.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- holmes/core/tracing.py
Contributor
Author
|
Thanks for the thorough review @Avi-Robusta! All feedback addressed in commits cb5da956 and fdb6bac9 : Your review comments: CodeRabbit nitpicks also addressed: |
esara
reviewed
Mar 23, 2026
esara
reviewed
Mar 23, 2026
esara
reviewed
Mar 23, 2026
Contributor
Author
|
Good point . I'll import the semantic convention constants from opentelemetry-semantic-conventions instead of hardcoding strings. The gen_ai.* attributes are stabilizing in the OTel semconv spec, so using the library ensures we stay in sync. |
henrikrexed
force-pushed
the
feat/otel-pr-clean
branch
from
March 30, 2026 12:24
ed4fb03 to
5c7d6ec
Compare
Contributor
Author
|
I made the changes on the semantic convention on my latest commit. Please review |
Avi-Robusta
previously approved these changes
Apr 6, 2026
Collaborator
There was a problem hiding this comment.
Great work, LGTM
It is blocked by DCO, can you look and follow the instructions there so we can merge?
https://github.com/HolmesGPT/holmesgpt/pull/1761/checks?check_run_id=69169643806
henrikrexed
force-pushed
the
feat/otel-pr-clean
branch
from
April 7, 2026 11:56
5c7d6ec to
ec2a3ec
Compare
Collaborator
|
Hey @henrikrexed, |
- Add OTel tracing with TracingFactory integration (otel_tracing.py) - Instrument LLM calls with gen_ai semantic conventions - Add per-tool spans with timing and error metrics - Add investigation count/duration/iterations metrics - Add OTel documentation and tests - Optional otel dependency group in pyproject.toml - Auto-enable OTel when OTEL_EXPORTER_OTLP_ENDPOINT is set Co-Authored-By: Paperclip <noreply@paperclip.ing> Signed-off-by: Henrik Rexed <henrik.rexed@dynatrace.com>
henrikrexed
force-pushed
the
feat/otel-pr-clean
branch
from
April 9, 2026 13:09
ec2a3ec to
8d843e9
Compare
Contributor
Author
|
I think I fixed most of the issues |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Tests referenced non-existent `llm_input_tokens` attr (actual: `token_usage`) and standalone `get_metrics()` function (actual: `TracingFactory.get_metrics()`). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Move trace_span.end() into the finally block so the span is closed even when ai.call() or metric emission raises an exception. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Test cases 255_mcp_anyof_union_flattening and 175_coralogix_metrics_frontend use the 'mcp' tag which was not registered in pyproject.toml. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Test case 238_image_compaction_overflow uses the 'images' tag which was not registered in pyproject.toml. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Reorder markers to match master exactly (images before elasticsearch, mcp at end without trailing comma). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Avi-Robusta
approved these changes
Apr 11, 2026
Avi-Robusta
left a comment
Avi-Robusta
left a comment
Collaborator
There was a problem hiding this comment.
fixed some rebase issues
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add comprehensive OpenTelemetry observability to HolmesGPT with distributed traces and metrics for investigations, LLM calls, and tool/MCP execution. Activates automatically via OTEL_EXPORTER_OTLP_ENDPOINT env var — zero overhead when disabled.
Distributed Traces
Every investigation produces a connected trace hierarchy:
holmesgpt.investigation (root span)
├── gen_ai.chat (LLM iteration 0 — includes token counts)
│ └── POST (auto-instrumented httpx → LLM provider)
├── holmesgpt.tool. (tool/MCP call)
│ └── POST (auto-instrumented httpx → MCP server)
│ └── MCP server spans (if OTel-enabled)
├── gen_ai.chat (LLM iteration 1)
└── gen_ai.chat (final iteration — produces answer)
• W3C traceparent propagation to MCP servers via httpx auto-instrumentation
• Thread-safe context propagation using contextvars.copy_context() for ThreadPoolExecutor
• Safe detach handling for cross-context generator yields
Metrics (8 instruments)
gen_ai.client.token.usage
• Type: Counter
• Dimensions: gen_ai_request_model , gen_ai_system , gen_ai_token_type
gen_ai.client.operation.duration
• Type: Histogram
• Dimensions: gen_ai_request_model , gen_ai_system
holmesgpt.investigation.count
• Type: Counter
• Dimensions: gen_ai_request_model
holmesgpt.investigation.duration
• Type: Histogram
• Dimensions: gen_ai_request_model
holmesgpt.investigation.iterations
• Type: Histogram
• Dimensions: gen_ai_request_model
holmesgpt.tool.call.count
• Type: Counter
• Dimensions: holmesgpt_tool_name
holmesgpt.tool.call.duration
• Type: Histogram
• Dimensions: holmesgpt_tool_name
holmesgpt.tool.call.errors
• Type: Counter
• Dimensions: holmesgpt_tool_name
Dimension keys use underscores for maximum backend compatibility (Dynatrace, Grafana, etc.).
Configuration
Helm values.yaml
additionalEnvVars:
value: "http://otel-collector.monitoring.svc:4317"
value: "grpc"
value: "holmesgpt"
When OTEL_EXPORTER_OTLP_ENDPOINT is not set, HolmesGPT uses a no-op DummyTracer with zero overhead. No flags or code changes required.
Changes
• holmes/core/otel_tracing.py (new) — OpenTelemetryTracer , OTelSpan , OTelMetrics classes with SDK setup, Views, and metric instruments
• holmes/core/tracing.py — TracingFactory that selects OpenTelemetryTracer or DummyTracer based on env
• holmes/core/tool_calling_llm.py — Instrumented agentic loop: LLM token/duration metrics, tool call count/duration/error metrics in both streaming and non-streaming paths
• server.py — Investigation count/duration/iterations metrics in /api/chat endpoint
• holmes/main.py — Initialize tracer at startup
• Dockerfile — Install OTel optional dependencies
• pyproject.toml / poetry.lock — OTel SDK dependencies ( opentelemetry-sdk , opentelemetry-exporter-otlp-proto-grpc , opentelemetry-instrumentation-httpx )
• docs/reference/opentelemetry.md (new) — Full documentation with config, metric reference, example queries (DQL + PromQL), and backend examples
• mkdocs.yml — Added OTel docs to nav
• tests/test_otel_tracing.py (new) — 21 unit tests covering tracer, spans, metrics, and factory
• holmes/plugins/prompts/_general_instructions.jinja2 — Fix {% if runbooks_enabled %} guard for deployments without runbooks
Testing
• 21 unit tests (all pass)
• Verified end-to-end: 195-span trace with 8 LLM iterations and 15+ MCP tool calls
• Tested with Dynatrace and OTel Collector backends
• Confirmed zero overhead when OTel is disabled
Summary by CodeRabbit
New Features
Documentation
Tests
Chores