Conversation
|
|
|
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:
WalkthroughAdds an experimental OpenTelemetry package (tracing, metrics, logging), wires OTEL into server and AG‑UI agent flows with per-run / per-iteration / per-tool spans and streaming events, extends TracingFactory for OTEL/composite tracers, and adds tests, docs, and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AGUI as AG-UI Server
participant Tracer as OTEL Tracer
participant Exporter as OTLP/OSIS Backend
Client->>AGUI: start chat / agent run
activate AGUI
AGUI->>Tracer: init_otel_tracer() (if enabled)
Tracer->>Exporter: configure exporter (OTLP or SigV4)
Tracer-->>AGUI: tracer ready
AGUI->>Tracer: start_span("agent_run", attrs...)
activate Tracer
loop each LLM iteration
AGUI->>Tracer: start_span("llm_iteration")
AGUI->>Tracer: end_span("llm_iteration", token/cost attrs)
alt tool required
AGUI->>Tracer: start_span("tool_execute", TOOL_NAME)
AGUI->>Tracer: end_span("tool_execute", duration/output/error)
end
end
alt success
AGUI->>Tracer: set attribute RESULT_SUCCESS=true
else error
AGUI->>Tracer: set_span_error(exception)
end
AGUI->>Tracer: end_span("agent_run")
Tracer->>Exporter: export / flush spans
AGUI-->>Client: stream/return response
deactivate AGUI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
experimental/otel/tracing.py (2)
158-159: Debug logging may expose sensitive headers.The Authorization header (containing AWS credentials signature) will be logged. While debug level, this could leak to log files in production if log levels are misconfigured.
🔎 Consider masking sensitive headers:
- # Debug: show signed headers - logging.debug(f"[OTEL SIGN] Headers after signing: {dict(aws_request.headers)}") + # Debug: show signed headers (mask Authorization) + debug_headers = {k: v if k.lower() != 'authorization' else '[MASKED]' + for k, v in aws_request.headers.items()} + logging.debug(f"[OTEL SIGN] Headers after signing: {debug_headers}")
351-360: Use attribute constants for consistency.
set_span_erroruses hardcoded"error.type"and"error.message"whileattributes.pydefinesERROR_TYPEandERROR_MESSAGEconstants. Using the constants ensures consistency.🔎 Apply this diff:
+from experimental.otel.attributes import ERROR_TYPE, ERROR_MESSAGE + def set_span_error(span: trace.Span, error: Exception) -> None: """Set error status and attributes on a span. Args: span: The span to set error on error: The exception that occurred """ span.set_status(Status(StatusCode.ERROR, str(error))) - span.set_attribute("error.type", type(error).__name__) - span.set_attribute("error.message", str(error)) + span.set_attribute(ERROR_TYPE, type(error).__name__) + span.set_attribute(ERROR_MESSAGE, str(error))experimental/otel/attributes.py (1)
55-72: Truncation result exceedsmax_size.The function truncates to
max_sizethen appends"...[TRUNCATED]"(14 chars), so the result ismax_size + 14bytes. If the goal is to enforce a strict size limit, account for the marker length.🔎 Strict size enforcement:
+TRUNCATION_MARKER = "...[TRUNCATED]" + def truncate(value: Optional[str], max_size: int = MAX_ATTRIBUTE_SIZE) -> str: if value is None: return "" if len(value) <= max_size: return value - return value[:max_size] + "...[TRUNCATED]" + return value[:max_size - len(TRUNCATION_MARKER)] + TRUNCATION_MARKERexperimental/ag-ui/server-agui.py (1)
21-29: Consider proper package structure over sys.path manipulation.The
sys.path.insertis fragile and can cause import issues. Since this is experimental code, it's acceptable, but consider adding the experimental directory as a proper package or using relative imports when stabilizing.experimental/otel/test_otel.py (2)
1-9: Consider converting to pytest format.The script works for quick verification but integrating with pytest would provide better CI/CD integration, fixtures for setup/teardown, and consistent test discovery.
113-117: Direct module state manipulation is fragile.Modifying
tracing._initializedand_tracer_providerdirectly works but is brittle. When converting to pytest, consider usingimportlib.reload()or providing a properreset()function in the tracing module for testing.experimental/otel/__init__.py (1)
1-51: Clean package API surface design.This module correctly aggregates and re-exports the public OTEL instrumentation API. The explicit
__all__makes the public interface clear.Optionally, consider sorting
__all__alphabetically for easier maintenance as the list grows (per static analysis hint RUF022).experimental/otel/test_otel_integration.py (2)
111-116: Fragile coupling to private module state.Directly resetting
tracing._initializedandtracing._tracer_providercouples this test to internal implementation details. If the module internals change (e.g., renamed variables, different state management), this test will break silently or require updates.Consider exposing a
reset_tracer()test utility in the tracing module if this pattern is needed elsewhere, or document this coupling with a comment.
255-261: Direct access to private_tracer_provider.Similar to
test_tracer_initialization, this accesses private module state. Consider whethershutdown_otel_tracer()(which is publicly exported) could be used, or expose aforce_flush()wrapper in the public API.The 10-second timeout for
force_flushis reasonable for integration tests.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 3f1d3a3 and 28dd21cd4ee9f45995101ca275ea1d4f8fb0d9ab.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
experimental/ag-ui/server-agui.py(8 hunks)experimental/otel/__init__.py(1 hunks)experimental/otel/attributes.py(1 hunks)experimental/otel/test_otel.py(1 hunks)experimental/otel/test_otel_integration.py(1 hunks)experimental/otel/tracing.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
experimental/otel/test_otel.py (3)
experimental/otel/tracing.py (1)
_extract_region_from_endpoint(71-90)experimental/otel/attributes.py (1)
truncate(55-72)holmes/core/tracing.py (2)
start_span(104-105)end(110-111)
experimental/otel/test_otel_integration.py (2)
experimental/otel/tracing.py (3)
init_otel_tracer(241-323)get_tracer(326-340)set_span_error(351-360)experimental/otel/attributes.py (1)
truncate(55-72)
experimental/ag-ui/server-agui.py (3)
experimental/otel/tracing.py (3)
init_otel_tracer(241-323)get_tracer(326-340)set_span_error(351-360)holmes/core/tracing.py (2)
start_span(104-105)end(110-111)experimental/otel/attributes.py (1)
truncate(55-72)
experimental/otel/__init__.py (2)
experimental/otel/tracing.py (4)
init_otel_tracer(241-323)get_tracer(326-340)shutdown_otel_tracer(343-348)set_span_error(351-360)experimental/otel/attributes.py (1)
truncate(55-72)
experimental/otel/tracing.py (1)
holmes/version.py (1)
get_version(48-130)
🪛 Ruff (0.14.8)
experimental/otel/test_otel.py
225-225: Do not catch blind exception: Exception
(BLE001)
experimental/otel/test_otel_integration.py
235-235: Abstract raise to an inner function
(TRY301)
235-235: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Do not catch blind exception: Exception
(BLE001)
264-264: Consider moving this statement to an else block
(TRY300)
265-265: Do not catch blind exception: Exception
(BLE001)
453-453: Do not catch blind exception: Exception
(BLE001)
485-485: Do not catch blind exception: Exception
(BLE001)
experimental/ag-ui/server-agui.py
294-294: Use explicit conversion flag
Replace with conversion flag
(RUF010)
experimental/otel/__init__.py
29-51: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
experimental/otel/tracing.py
88-89: try-except-pass detected, consider logging the exception
(S110)
88-88: Do not catch blind exception: Exception
(BLE001)
106-106: Avoid specifying long messages outside the exception class
(TRY003)
234-234: Consider moving this statement to an else block
(TRY300)
318-318: Consider moving this statement to an else block
(TRY300)
369-369: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (16)
experimental/otel/tracing.py (2)
181-213: Thread-safety concern is well-documented.The environment variable manipulation for profile isolation is a known limitation. The documentation is clear about startup-only usage. Consider adding an assertion or runtime check to prevent accidental concurrent calls if this becomes a concern.
264-272: Initialization semantics are correct.Setting
_initialized = Trueeven when disabled or missing endpoint is intentional — it prevents repeated initialization attempts on subsequentget_tracer()calls. The early returns withFalsecorrectly indicate tracing won't be active.experimental/otel/attributes.py (1)
8-48: Well-structured attribute constants.The constants follow Gen AI semantic conventions and are clearly organized by category. Good documentation linking to AG-UI field mappings.
experimental/ag-ui/server-agui.py (4)
85-91: LGTM - Tracer initialization at startup.Module-level initialization ensures tracing is configured before handling requests. The
get_tracer()call safely returns a no-op tracer when disabled.
142-153: LGTM - Root span with correlation attributes.Creating the span inside the generator ensures it covers the streaming lifecycle. The correlation attributes (REQUEST_ID, CONVERSATION_ID) properly link traces to AG-UI request/thread identifiers.
208-229: LGTM - Tool execution spans with proper parent-child linking.The implementation correctly:
- Links child spans to the root using
trace.set_span_in_context- Calculates duration from tracked start times
- Truncates potentially large tool outputs
- Handles missing start times gracefully with
pop(..., None)
288-298: LGTM - Robust error handling and span lifecycle.The
finallyblock ensures the root span is always ended, preventing resource leaks. Error details are properly recorded on the span before the error event is yielded.experimental/otel/test_otel.py (2)
40-71: Good edge case coverage for truncate.Tests cover all key scenarios: None input, strings within limit, at exact limit, over limit, and empty strings. The assertions include helpful failure messages.
220-228: Test runner correctly aggregates failures.The broad
Exceptioncatch is intentional here to ensure all tests run even if one fails. Consider addingtraceback.print_exc()for better debugging of failures.experimental/otel/test_otel_integration.py (7)
1-28: Good documentation for integration test usage.The docstring clearly documents all required and optional environment variables, usage instructions, and the purpose of the test. This helps developers run the test correctly.
69-73: Good practice: Masking sensitive credentials in output.The password is correctly masked with
"***"to prevent accidental exposure in logs/output.
180-194: Correct span hierarchy and context propagation.Child tool spans are properly created with
trace.set_span_in_context(root_span)ensuring correct parent-child relationships in the trace. Each tool span is correctly ended within the loop.
230-243: Intentional error simulation for testing.The broad
Exceptioncatch and intentionalValueErrorraise are appropriate here—this is a test exercising the error-recording path. The finally block correctly ensures the span is ended regardless of outcome.
344-349: Good retry pattern with exponential backoff potential.The retry loop with configurable
max_retriesandretry_delayis appropriate for handling indexing delays. All HTTP requests correctly usetimeout=10to prevent hangs.
489-500: Good conditional verification with credential check.The OpenSearch verification is correctly gated behind a check for all three required variables (
OPENSEARCH_ENDPOINT,OPENSEARCH_USERNAME,OPENSEARCH_PASSWORD), and failure doesn't break the overall test result—appropriate for optional verification that may have indexing delays.
273-276: Credentials sourced from environment variables.OpenSearch credentials are correctly read from environment variables rather than hardcoded. Empty string defaults are appropriate since the verification is optional.
|
@goyamegh Is it possible to instead add this to existing tracing module so it's pluggable like Braintrust tracer? holmesgpt/holmes/core/tracing.py Line 155 in b57d18b Relevant PR: #1331 |
|
Agree with the comment from @kylehounslow - I think that's the best way to do this. |
28dd21c to
264cb68
Compare
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@experimental/ag-ui/server-agui.py`:
- Around line 189-191: The variable tool_start_times is annotated as dict[str,
float] but is being used to store span objects; create a separate dict (e.g.,
active_spans: dict[str, Span]) to hold span objects and keep tool_start_times
strictly for float timestamps, update the initialization to: tool_start_times:
dict[str, float] = {} and active_spans: dict[str, Span] = {}, then replace every
place that stores or retrieves spans (keys like "invoke_{tool_call_id}",
"parse_response", etc.) to use active_spans while leaving timestamp calculations
on tool_start_times so type hints and usages match (adjust imports/types if
needed and update code references in functions that start/finish spans or
compute durations such as start/invoke/finish handlers).
- Around line 536-566: Exception paths currently only set errors and end
current_chat_span and root_span, leaving spans held in tool_start_times (keys
like "invoke", "parse_response", "context_check", "error_handling") unclosed;
ensure you iterate over tool_start_times in the finally block and for each span
call set_span_error(span, e) if an exception occurred (or mark success
otherwise) and end() the span, and also clear or reset tool_start_times after
closing; update finally to close all spans (referencing tool_start_times,
set_span_error, current_chat_span, root_span) so no spans remain open on error
or normal exit.
In `@holmes/core/tool_calling_llm.py`:
- Around line 783-789: The tokens_used value is computed incorrectly; instead of
subtracting maximum_output_token from max_context_size, use the actual consumed
tokens from limit_result.tokens.total_tokens (or a safe fallback if tokens or
total_tokens is missing). Update the build_stream_event_context_check call in
the block using limit_result.tokens.total_tokens for tokens_used, keep
tokens_limit as limit_result.max_context_size (with the existing hasattr
checks), and ensure you handle missing attributes without raising (e.g., None
fallback).
In `@pyproject.toml`:
- Around line 58-60: The OpenTelemetry dependencies (opentelemetry-api,
opentelemetry-sdk, opentelemetry-exporter-otlp-proto-http) are pinned to
^1.29.0; update each dependency version specifier to ^1.39.1 in pyproject.toml
so the project uses the latest stable release (replace the current ^1.29.0
entries for opentelemetry-api, opentelemetry-sdk, and
opentelemetry-exporter-otlp-proto-http with ^1.39.1).
🧹 Nitpick comments (26)
holmes/config.py (1)
175-231: LGTM! Clear file-then-env merge strategy.The implementation correctly:
- Loads from
~/.holmes/config.yamlif present- Overlays environment variable overrides
- Uses
model_dump()for Pydantic v2 compatibilityOne minor inconsistency:
load_from_file(line 166) uses the deprecated.dict()method while this method uses.model_dump(). Consider updating line 166 for consistency.docs/otel-tracing.md (2)
7-11: Add blank line before list for proper MkDocs rendering.As per coding guidelines, MkDocs requires a blank line between header/bold text and a list for proper rendering.
📝 Proposed fix
HolmesGPT supports comprehensive observability through OpenTelemetry, enabling: + - **Distributed Tracing**: Track requests across the entire agent execution lifecycle - **Metrics**: Monitor token usage, operation durations, and error rates - **Structured Logging**: Correlate logs with trace context
254-266: Consider using proper headings instead of bold emphasis for issue titles.The troubleshooting subsections use bold text (
**"error message"**) which markdownlint flags as emphasis-as-heading. Using####headings would improve navigation and accessibility.📝 Proposed fix
### Common Issues -**"OTEL tracing requested but OTEL_ENABLED not set to 'true'"** +#### "OTEL tracing requested but OTEL_ENABLED not set to 'true'" Set `OTEL_ENABLED=true` in your environment. -**"Failed to create OSIS session"** +#### "Failed to create OSIS session" Check your AWS credentials and ensure `OTEL_AWS_PROFILE` points to a valid profile with OSIS permissions. -**"payload too large" errors** +#### "payload too large" errors Tool outputs are automatically truncated to 8KB. If you still see this error, check for large metadata values.experimental/otel/tracing.py (4)
71-96:LoggingSpanExporterdoesn't inherit fromSpanExporter, causing type mismatch at line 369.The wrapper class doesn't implement the
SpanExporterinterface, which means passing it toBatchSpanProcessor(line 376-381) relies on duck typing. This works but loses type safety.♻️ Proposed fix
+from opentelemetry.sdk.trace.export import SpanExporter + -class LoggingSpanExporter: +class LoggingSpanExporter(SpanExporter): """Wrapper around SpanExporter that logs export results.""" def __init__(self, wrapped_exporter: OTLPSpanExporter): self._wrapped = wrapped_exporter
98-110: Consider consolidating duplicate helper functions into a shared module.
_get_otel_enabled()and_get_otel_endpoint()are duplicated acrosstracing.py,metrics.py, andotel_logging.py. This violates DRY and creates maintenance burden.Consider creating
experimental/otel/config.pyto centralize environment variable access.
138-158: Log exceptions instead of silently passing.The
exceptblock silently swallows all exceptions, making debugging difficult when region extraction fails unexpectedly.♻️ Proposed fix
try: parsed = urlparse(endpoint) if not parsed.hostname: logging.warning( f"[OTEL] Could not parse hostname from endpoint: {endpoint}" ) return "us-east-1" host_parts = parsed.hostname.split(".") # OSIS endpoints look like: xxx.<region>.osis.amazonaws.com for i, part in enumerate(host_parts): if part == "osis" and i > 0: return host_parts[i - 1] - except Exception: - pass + except Exception as e: + logging.debug(f"[OTEL] Failed to extract region from endpoint: {e}") return "us-east-1" # Default fallback
252-284: Thread-safety risk acknowledged but consider alternative approach.The env var manipulation creates a TOCTOU race condition. While the warning is well-documented, consider using
botocore.session.Sessiondirectly with explicit credential configuration instead of clearing env vars:from botocore.session import Session as BotocoreSession session = BotocoreSession() session.set_config_variable('profile', otel_profile) boto_session = boto3.Session(botocore_session=session)This avoids env var manipulation entirely.
experimental/otel/metrics.py (1)
77-91: Fragile endpoint detection using string matching.The check
".osis." in endpoint or ".es." in endpointis brittle and may fail for:
- Custom OSIS domains
- Endpoints with "osis" or "es" in the path (false positive)
- Future AWS endpoint format changes
Consider using the same
OTEL_AWS_PROFILEcheck as the primary signal, or add an explicitOTEL_AWS_SIGV4_ENABLEDenv var.♻️ Proposed fix
- if aws_profile or ".osis." in endpoint or ".es." in endpoint: + # Use explicit env var or profile presence to determine SigV4 need + use_sigv4 = aws_profile or os.environ.get("OTEL_AWS_SIGV4_ENABLED", "false").lower() == "true" + if use_sigv4:holmes/utils/stream.py (1)
271-274: Consider truncating theerrorfield for consistency.The
resultfield is truncated at 8192 characters, buterroris not. Thebuild_stream_event_error_handlingfunction truncateserror_messageat 1024. Consider applying similar truncation toerrorhere to prevent large payloads from verbose error messages.♻️ Suggested fix
if error is not None: - data["error"] = error + data["error"] = error[:1024] if len(error) > 1024 else errorserver.py (1)
166-166: Performance: Create tracer once at module level, not per request.
TracingFactory.create_tracer("otel")is called on every request. The tracer should be created once at startup and reused. This avoids repeated lookups and object creation on the hot path.♻️ Suggested fix
otel_enabled = init_otel() +otel_tracer = TracingFactory.create_tracer("otel") if otel_enabled else None config = Config.load_from_env()Then in the middleware:
- tracer = TracingFactory.create_tracer("otel") + tracer = otel_tracerexperimental/ag-ui/server-agui.py (2)
24-26: Consider avoidingsys.pathmanipulation.Manipulating
sys.pathis a code smell that can lead to import issues and makes the module harder to maintain. Consider:
- Installing the experimental module as a package
- Using relative imports if the module structure allows
- Adding the experimental path to
PYTHONPATHin the deployment configuration
562-562: Style: Use explicit conversion flag instead ofstr().Per Ruff RUF010, prefer
{e!s}over{str(e)}for string conversion in f-strings.♻️ Suggested fix
- message=f"Agent encountered an error: {str(e)}", + message=f"Agent encountered an error: {e!s}",holmes/core/tool_calling_llm.py (1)
966-966: Consider tracking actual duration or omittingduration_ms.
duration_ms=0is always emitted which is misleading. The comment says duration is tracked separately, but emitting a constant 0 pollutes the trace data. Either track the actual duration betweentool_invoke_startandtool_invoke_end, or omit the field.♻️ Option 1: Track duration
Store the start time when emitting
tool_invoke_start:tool_start_time = time.time() yield build_stream_event_tool_invoke_start(...) # ... tool execution ... duration_ms = int((time.time() - tool_start_time) * 1000) yield build_stream_event_tool_invoke_end(..., duration_ms=duration_ms, ...)♻️ Option 2: Make duration optional
Modify
build_stream_event_tool_invoke_endto makeduration_msoptional and omit it when not tracked.experimental/otel/test_otel_integration.py (5)
38-41: Consider using a more robust path manipulation approach.The
sys.path.insert(0, ...)pattern can lead to import order issues. For test files, consider using pytest with proper package configuration or a conftest.py that handles path setup.
111-116: Direct manipulation of private module state is fragile.Resetting
_initializedand_tracer_providerdirectly couples this test to internal implementation details. If the module's internals change, these tests will break silently.Consider adding a public
reset_tracer()function to the tracing module for test purposes, or use importlib to reload the module cleanly.
129-130: Global mutable state for test coordination.Using a module-level global
_current_test_idmakes tests harder to parallelize and can cause subtle bugs if test order changes.Consider returning the test_id from
test_create_and_export_spans()and passing it explicitly toverify_traces_in_opensearch().
255-255: Accessing private variable from another module.
_tracer_provideris a private module variable. This tight coupling could break if the tracing module's internals change.Consider exposing a public
force_flush()function in the tracing module API instead.♻️ Suggested approach
Add to
experimental/otel/tracing.py:def force_flush(timeout_millis: int = 10000) -> bool: """Force flush all pending spans to the backend.""" if _tracer_provider: return _tracer_provider.force_flush(timeout_millis=timeout_millis) return FalseThen in the test:
-from experimental.otel.tracing import _tracer_provider +from experimental.otel.tracing import force_flush -if _tracer_provider: - _tracer_provider.force_flush(timeout_millis=10000) +force_flush(timeout_millis=10000)
341-341: Credentials passed in plain tuple - consider auth abstraction.While functional, passing credentials as a tuple
(username, password)is common but consider usingrequests.auth.HTTPBasicAuthexplicitly for clarity.experimental/otel/test_otel.py (3)
7-8: Same sys.path manipulation concern as integration test.Consider a shared test utility or conftest.py for path setup.
151-158: Repeated pattern of manipulating internal module state.Multiple tests reset
_initialized,_tracer_provider,_meter,_meter_providerdirectly. This creates maintenance burden and tight coupling to implementation details.Consider creating a test utilities module with reset functions, or exposing test hooks in the OTEL modules.
Also applies to: 271-281
571-594: Fragile tests that parse source files.These tests read
server.pyandserver-agui.pyas text and check for exact string matches. This approach:
- Breaks if imports are reformatted (e.g., multi-line imports)
- Doesn't verify actual runtime behavior
- Creates a brittle dependency on code formatting
Consider using AST parsing or importlib inspection instead, or simply rely on actual integration tests that exercise the code paths.
♻️ Alternative using AST
import ast def test_server_uses_tracing_factory(): """Test that server.py uses TracingFactory correctly.""" print("Testing server.py TracingFactory usage...") server_path = os.path.join( os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))), "server.py" ) with open(server_path, "r") as f: tree = ast.parse(f.read()) # Check for TracingFactory import imports = [node for node in ast.walk(tree) if isinstance(node, ast.ImportFrom)] has_tracing_import = any( node.module == "holmes.core.tracing" and any(alias.name == "TracingFactory" for alias in node.names) for node in imports ) assert has_tracing_import, "TracingFactory not imported in server.py" print(" ✅ server.py imports TracingFactory")Also applies to: 597-618
experimental/otel/otel_logging.py (3)
39-41: Duplicated_get_otel_enabled()function across modules.This function is identical in
tracing.py,metrics.py, andotel_logging.py. Consider extracting to a shared config module to follow DRY principles.♻️ Suggested refactor
Create
experimental/otel/config.py:"""Shared OTEL configuration helpers.""" import os def get_otel_enabled() -> bool: """Check if OTEL is enabled via environment variable.""" return os.environ.get("OTEL_ENABLED", "false").lower() == "true"Then import from there in all modules.
49-51: Unused function_get_log_content_enabled().This function is defined but never called in this file or exported. Either use it in the logging helpers to conditionally include content, or remove it.
246-249: Use f-string conversion flag for clarity.Per static analysis, use explicit conversion flag instead of
str(error).♻️ Suggested fix
logger.error( - f"Agent error: {agent_name} error_type={err_type} message={str(error)}", + f"Agent error: {agent_name} error_type={err_type} message={error!s}", exc_info=True, )holmes/core/tracing.py (1)
325-336: Unusedtypeparameter inset_attributes.The
typeparameter is accepted but never used. If this is for API compatibility, consider documenting it or logging when it's provided but ignored.experimental/otel/__init__.py (1)
115-222: Consider sorting__all__for consistency.Per static analysis (RUF022), the
__all__list is not sorted. While the current grouping by category is logical and readable, sorting would help with automated tooling and make diffs cleaner when adding new exports.This is a minor style suggestion - the current categorical organization has its own merits for readability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 28dd21cd4ee9f45995101ca275ea1d4f8fb0d9ab and 264cb684f12bd8c14f8b1b472d3da767ed9dbfca.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
docs/otel-tracing.mdexperimental/ag-ui/server-agui.pyexperimental/otel/__init__.pyexperimental/otel/attributes.pyexperimental/otel/metrics.pyexperimental/otel/otel_logging.pyexperimental/otel/test_otel.pyexperimental/otel/test_otel_integration.pyexperimental/otel/tracing.pyholmes/config.pyholmes/core/tool_calling_llm.pyholmes/core/tracing.pyholmes/utils/stream.pypyproject.tomlserver.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
holmes/config.pyexperimental/otel/tracing.pyexperimental/ag-ui/server-agui.pyholmes/core/tool_calling_llm.pyexperimental/otel/test_otel.pyexperimental/otel/test_otel_integration.pyexperimental/otel/otel_logging.pyserver.pyholmes/core/tracing.pyexperimental/otel/__init__.pyholmes/utils/stream.pyexperimental/otel/attributes.pyexperimental/otel/metrics.py
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use semantic, descriptive names for variables, functions, and components
Write clear, concise comments that explain 'why' rather than 'what'
Files:
holmes/config.pyexperimental/otel/tracing.pyexperimental/ag-ui/server-agui.pyholmes/core/tool_calling_llm.pyexperimental/otel/test_otel.pyexperimental/otel/test_otel_integration.pyexperimental/otel/otel_logging.pyserver.pyholmes/core/tracing.pyexperimental/otel/__init__.pyholmes/utils/stream.pyexperimental/otel/attributes.pyexperimental/otel/metrics.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Add blank line between header/bold text and a list in MkDocs documentation files, otherwise lists won't render properly
Files:
docs/otel-tracing.md
🧠 Learnings (1)
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.{py,yaml} : All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message)
Applied to files:
holmes/core/tool_calling_llm.py
🧬 Code graph analysis (9)
holmes/config.py (1)
holmes/utils/pydantic_utils.py (1)
load_model_from_file(39-54)
experimental/otel/tracing.py (2)
experimental/otel/otel_logging.py (2)
format(21-36)_get_otel_enabled(39-41)experimental/otel/metrics.py (2)
_get_otel_enabled(31-33)_get_otel_endpoint(41-43)
experimental/ag-ui/server-agui.py (5)
holmes/core/tracing.py (8)
TracingFactory(510-599)init_otel(516-532)start_span(117-118)start_span(297-305)start_span(442-447)end(123-124)end(338-340)end(459-462)experimental/otel/tracing.py (2)
get_tracer(409-423)set_span_error(434-445)experimental/otel/metrics.py (8)
init_otel_metrics(46-162)shutdown_otel_metrics(170-180)record_token_usage(186-210)record_operation_duration(213-240)record_tool_duration(243-264)increment_iterations(267-288)increment_tool_calls(291-312)increment_errors(315-336)holmes/utils/stream.py (1)
StreamEvents(17-37)experimental/otel/attributes.py (1)
truncate(159-177)
holmes/core/tool_calling_llm.py (2)
holmes/utils/stream.py (7)
build_stream_event_llm_iteration_start(166-182)build_stream_event_llm_iteration_complete(185-219)build_stream_event_tool_invoke_start(222-244)build_stream_event_tool_invoke_end(247-278)build_stream_event_parse_response(281-300)build_stream_event_context_check(303-326)build_stream_event_error_handling(329-351)holmes/core/truncation/input_context_window_limiter.py (1)
limit_input_context_window(147-219)
experimental/otel/test_otel_integration.py (3)
experimental/otel/otel_logging.py (1)
format(21-36)experimental/otel/tracing.py (3)
init_otel_tracer(312-406)get_tracer(409-423)set_span_error(434-445)experimental/otel/attributes.py (1)
truncate(159-177)
experimental/otel/otel_logging.py (3)
experimental/otel/tracing.py (1)
_get_otel_enabled(98-100)experimental/otel/metrics.py (1)
_get_otel_enabled(31-33)holmes/plugins/toolsets/logging_utils/logging_api.py (1)
logger_name(82-83)
server.py (1)
holmes/core/tracing.py (11)
TracingFactory(510-599)SpanType(103-111)init_otel(516-532)create_tracer(568-599)start_trace(145-147)start_trace(197-227)start_trace(391-416)start_trace(487-492)set_attributes(126-129)set_attributes(325-336)set_attributes(454-457)
experimental/otel/__init__.py (4)
experimental/otel/tracing.py (4)
init_otel_tracer(312-406)get_tracer(409-423)shutdown_otel_tracer(426-431)set_span_error(434-445)experimental/otel/attributes.py (1)
truncate(159-177)experimental/otel/metrics.py (9)
init_otel_metrics(46-162)get_meter(165-167)shutdown_otel_metrics(170-180)record_token_usage(186-210)record_operation_duration(213-240)record_tool_duration(243-264)increment_iterations(267-288)increment_tool_calls(291-312)increment_errors(315-336)experimental/otel/otel_logging.py (3)
OTELContextFormatter(14-36)setup_otel_logging(54-92)get_otel_logger(95-104)
experimental/otel/metrics.py (2)
experimental/otel/tracing.py (4)
export(77-89)_get_otel_enabled(98-100)_get_otel_endpoint(108-110)_create_osis_session(240-309)experimental/otel/otel_logging.py (1)
_get_otel_enabled(39-41)
🪛 LanguageTool
docs/otel-tracing.md
[style] ~266-~266: Consider an alternative verb to strengthen your wording.
Context: ...atically truncated to 8KB. If you still see this error, check for large metadata va...
(IF_YOU_HAVE_THIS_PROBLEM)
🪛 markdownlint-cli2 (0.18.1)
docs/otel-tracing.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
85-85: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
256-256: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
260-260: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
264-264: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.11)
experimental/otel/tracing.py
155-156: try-except-pass detected, consider logging the exception
(S110)
155-155: Do not catch blind exception: Exception
(BLE001)
173-173: Avoid specifying long messages outside the exception class
(TRY003)
305-305: Consider moving this statement to an else block
(TRY300)
401-401: Consider moving this statement to an else block
(TRY300)
454-454: Do not catch blind exception: Exception
(BLE001)
experimental/ag-ui/server-agui.py
562-562: Use explicit conversion flag
Replace with conversion flag
(RUF010)
experimental/otel/test_otel.py
231-231: Possible hardcoded password assigned to: "METRIC_TOKEN_USAGE"
(S105)
654-654: Do not catch blind exception: Exception
(BLE001)
experimental/otel/test_otel_integration.py
235-235: Abstract raise to an inner function
(TRY301)
235-235: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Do not catch blind exception: Exception
(BLE001)
264-264: Consider moving this statement to an else block
(TRY300)
265-265: Do not catch blind exception: Exception
(BLE001)
453-453: Do not catch blind exception: Exception
(BLE001)
485-485: Do not catch blind exception: Exception
(BLE001)
experimental/otel/otel_logging.py
248-248: Use explicit conversion flag
Replace with conversion flag
(RUF010)
server.py
182-182: Consider moving this statement to an else block
(TRY300)
holmes/core/tracing.py
298-298: Unused method argument: kwargs
(ARG002)
312-312: Unused method argument: kwargs
(ARG002)
328-328: Unused method argument: type
(ARG002)
380-380: Unused method argument: experiment_name
(ARG002)
381-381: Unused method argument: additional_metadata
(ARG002)
529-529: Consider moving this statement to an else block
(TRY300)
530-530: Do not catch blind exception: Exception
(BLE001)
experimental/otel/__init__.py
115-222: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
experimental/otel/attributes.py
142-142: Possible hardcoded password assigned to: "METRIC_TOKEN_USAGE"
(S105)
150-150: Possible hardcoded password assigned to: "TOKEN_TYPE_INPUT"
(S105)
151-151: Possible hardcoded password assigned to: "TOKEN_TYPE_OUTPUT"
(S105)
experimental/otel/metrics.py
158-158: Consider moving this statement to an else block
(TRY300)
160-160: Do not catch blind exception: Exception
(BLE001)
177-177: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (31)
experimental/otel/tracing.py (2)
160-238: LGTM! Robust SigV4 signing implementation.The
AWSSigV4Sessionclass correctly:
- Handles credential refresh for assumed roles
- Separates
sendkwargs fromRequestkwargs- Computes content SHA256 hash for SigV4 signing
- Masks Authorization header in debug logs
312-407: LGTM! Well-structured initialization with proper lifecycle management.Good practices observed:
- Reads env vars at call time, not import time
- Idempotent initialization (checks
_initialized)- Registers
atexithandler for cleanup- Reduced batch size to prevent payload errors
Note: Setting
_initialized = Trueon failure paths (lines 337-338, 342-343, 405) is intentional to prevent repeated initialization attempts, which is a reasonable design choice.experimental/otel/metrics.py (1)
186-336: LGTM! Well-designed metric recording helpers.Good practices:
- Null guards prevent errors when metrics are disabled
- Consistent attribute naming via
otel_attrmodule- Clear docstrings explaining parameters
- Follows Gen AI semantic conventions
holmes/utils/stream.py (3)
26-37: LGTM - Well-structured event types for OTEL tracing.The new enum members follow the existing naming convention and provide clear categorization with helpful comments distinguishing LLM iteration events from granular span events.
166-219: LGTM - Clean iteration event builders.Both
build_stream_event_llm_iteration_startandbuild_stream_event_llm_iteration_completeare well-documented with appropriate optional parameters for finish_reason and cost_usd.
281-351: LGTM - Consistent start/end event pattern.The
is_startpattern for parse_response, context_check, and error_handling events provides a clean API. Lightweight start events and data-rich end events follow good observability practices.experimental/otel/attributes.py (3)
159-177: LGTM - Correct truncation implementation.The function properly accounts for the marker length when truncating, ensuring the final output stays within
max_size. HandlingNoneby returning an empty string is a sensible default.
142-151: Static analysis false positives - ignore S105 hints.The S105 warnings for
METRIC_TOKEN_USAGE,TOKEN_TYPE_INPUT, andTOKEN_TYPE_OUTPUTare false positives. These are OTEL metric/attribute name constants, not passwords or secrets.
14-31: Note: Using incubating OTEL semantic conventions.The import from
opentelemetry.semconv._incubating.attributes.gen_ai_attributesuses the incubating (unstable) API. Gen AI semantic conventions remain in Development status and may change in future OpenTelemetry releases.server.py (2)
67-75: LGTM - Clean OTEL initialization with proper error handling.The
init_otel()function follows the pattern requested in PR comments by usingTracingFactory.init_otel(). The opt-in viaOTEL_ENABLEDenvironment variable is appropriately guarded with logging for both success and failure cases.
177-191: LGTM - Correct exception handling with span error recording.The middleware properly records error attributes on the span before re-raising exceptions, ensuring errors are traced while preserving the original exception behavior.
experimental/ag-ui/server-agui.py (1)
96-106: LGTM - Proper OTEL initialization via TracingFactory.The initialization correctly uses
TracingFactory.init_otel()as requested in PR comments, making the OTEL integration pluggable. Both tracing and metrics initialization are properly logged.holmes/core/tool_calling_llm.py (4)
63-69: LGTM - Clean imports for new stream event builders.The imports are properly organized and follow the existing pattern in the file.
799-830: LGTM - Correct LLM iteration event emission.Token usage and cost extraction properly handle missing attributes with safe
getattrcalls and reuse the existing_extract_cost_from_responsehelper.
833-848: LGTM - Proper error handling event emission.The error handling events correctly emit start/end pairs with appropriate
will_retryflags distinguishing recoverable errors (IncorrectToolCall) from terminal errors (BadRequestError).
897-903: Verify:finish_reasonsscope in parse_response_end event.
finish_reasonsis assigned inside thetryblock (line 821). If the code reaches the parse_response_end emission after any error path that doesn't raise,finish_reasonsmight reference a stale value from a previous iteration or be undefined.Consider assigning
finish_reasons = Nonebefore the try block for safety.experimental/otel/test_otel_integration.py (2)
307-323: Previous review feedback addressed.The overly broad wildcard query has been removed as noted in the comment on line 308. The query now correctly relies on specific match clauses for the test_id.
461-514: Well-structured test orchestration.The
main()function properly checks environment, runs tests sequentially, handles failures gracefully, and provides clear summary output. The OpenSearch verification being non-blocking (line 495-498) is a good choice given indexing delays.experimental/otel/test_otel.py (2)
82-114: Thorough test coverage for truncate function.Good edge case coverage including None, empty string, exact limit, and over limit scenarios.
547-568: Good verification of SpanType to OTEL mappings.The test validates that all SpanTypes have OTEL mappings and that specific mappings follow Gen AI semantic conventions.
experimental/otel/otel_logging.py (2)
54-92: Well-designed logger setup with OTEL context.Good implementation that:
- Avoids duplicate handlers
- Conditionally adds trace context based on OTEL enablement
- Falls back to standard formatting when OTEL is disabled
26-34: No changes needed. The code correctly usesctx.trace_flags.sampled, which is the proper OpenTelemetry Python API for accessing the sampled flag from a SpanContext. No inconsistency exists.Likely an incorrect or invalid review comment.
holmes/core/tracing.py (8)
278-286: Good mapping of SpanTypes to Gen AI semantic conventions.The
SPAN_TYPE_TO_OTELmapping properly aligns internal span types with OpenTelemetry Gen AI semantic conventions, enabling consistent observability across backends.
297-305: Unused**kwargsparameter is acceptable for API compatibility.The
kwargsinstart_spanmaintains interface compatibility with Braintrust spans that may pass additional parameters. This is a reasonable design choice.
345-350: Good error handling in context manager exit.Properly records errors on the span before ending when an exception occurs. The import inside the method keeps the dependency lazy.
361-368: Lazy initialization is a good pattern.The
_ensure_initialized()method defers OTEL setup until first use, which is efficient for cases where tracing may not be needed.
467-469: CompositeSpan.exit handles exceptions differently than OTELSpan.
OTELSpan.__exit__callsset_span_errorwhenexc_valis present, butCompositeSpan.__exit__delegates to each span's__exit__. This is correct since each underlying span handles its own error recording.
524-532: Broad exception catch with appropriate fallback.While static analysis flags this as
BLE001, catching all exceptions during initialization is reasonable here since we want tracing to be non-blocking. The warning log and False return provide visibility into failures.
556-562: Good check for OTEL enablement before creating tracer.The explicit check for
OTEL_ENABLED=truewith a warning message provides clear feedback when OTEL is requested but not configured.
586-597: Well-designed composite tracer creation.The logic correctly:
- Creates tracers for each type
- Filters out DummyTracers
- Returns single tracer if only one is active
- Returns CompositeTracer only when multiple tracers are active
This addresses the PR objective of making OTEL pluggable alongside Braintrust.
experimental/otel/__init__.py (1)
1-113: Well-organized public API surface.The re-exports are logically grouped by category (tracing, attributes, metrics, logging) with clear comments. This provides a clean, centralized import point for consumers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
264cb68 to
af146b0
Compare
|
|
||
| ## AWS OSIS Integration | ||
|
|
||
| For AWS OpenSearch Ingestion Service (OSIS), the implementation: |
There was a problem hiding this comment.
nit: please hyperlink to AWS OSIS docs: https://docs.aws.amazon.com/opensearch-service/latest/developerguide/ingestion.html
| def truncate(value: Optional[str], max_size: int = MAX_ATTRIBUTE_SIZE) -> str: | ||
| """Truncate a string value to prevent OTEL payload size errors. | ||
|
|
||
| Based on ml-commons AgentTracer.truncate() pattern. |
There was a problem hiding this comment.
nit: ml-commons is not widely known. Please permalink to AgentTracer.truncate() method in ml-commons repo.
| # Create OTLP HTTP exporter with AWS SigV4 auth for OSIS | ||
| # Endpoint should be like: https://your-osis-pipeline/v1/traces | ||
| osis_session = _create_osis_session(otel_endpoint) | ||
| if osis_session: | ||
| exporter = OTLPSpanExporter(endpoint=otel_endpoint, session=osis_session) | ||
| else: | ||
| # Fall back to unauthenticated exporter (may fail with OSIS) | ||
| logging.warning( | ||
| "OSIS session creation failed, using unauthenticated exporter" | ||
| ) | ||
| exporter = OTLPSpanExporter(endpoint=otel_endpoint) |
There was a problem hiding this comment.
Can we consolidate how we're detecting unauthenticated OTLP endpoint versus AWS (OSIS) across metrics/logs/traces? Seems metrics has different logic: https://github.com/goyamegh/holmesgpt/blob/af146b07c4014b90459228b0bb2a44372a73db5f/experimental/otel/metrics.py#L79-L81
Idea: Centralize logic in otel/__init__.py instead
| @@ -0,0 +1,250 @@ | |||
| """OpenTelemetry correlated logging for HolmesGPT. | |||
There was a problem hiding this comment.
- Are these logs written to OTLP endpoint like metrics and traces?
- Should we name this file
logging.pyfor consistency withmetrics.pyandtracing.py?
| @@ -0,0 +1,222 @@ | |||
| """OpenTelemetry instrumentation for HolmesGPT experimental endpoints.""" | |||
There was a problem hiding this comment.
Since the tracing implementation is already integrated into core/, should we consider moving this out of experimental/?. My understanding is that experimental code should be removable without impacting the core codebase. What do you think about refactoring this to make OTel/OTLP export purely optional through feature flags (env vars)? This way, when the flags are disabled, no experimental code paths would execute.
| @app.middleware("http") | ||
| async def otel_tracing_middleware(request: Request, call_next): |
There was a problem hiding this comment.
Nice usage of middleware here! Are metrics/logs boilerplate already handled elsewhere?
| config_file = Path(DEFAULT_CONFIG_LOCATION) | ||
| config_from_file: Optional[Config] = None | ||
| if config_file.exists(): | ||
| logging.debug(f"Loading config from {config_file}") |
There was a problem hiding this comment.
nit: Since this is only logged once, consider INFO log-level.
|
nit: please update PR title to reflect latest changes. Perhaps just |
af146b0 to
aabfcf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
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)
1041-1068:⚠️ Potential issue | 🟡 MinorBug:
parse_response_startemitted without matchingparse_response_endonincorrect_tool_callpath.When
incorrect_tool_callis true (Line 1052), thecontinueon Line 1068 jumps back to the top of the while loop. ThePARSE_RESPONSE_STARTevent at Line 1042 is emitted but the matchingPARSE_RESPONSE_ENDat Line 1089 is skipped. This creates an unpaired start event in the stream, which will confuse any OTEL consumer that expects matched start/end pairs.🐛 Proposed fix: emit parse_response_end before continuing
if incorrect_tool_call: logging.warning( "Detected incorrect tool call. Structured output will be disabled. This can happen on models that do not support tool calling. For Azure AI, make sure the model name contains 'gpt-4.1' or other structured output compatible models. To disable this holmes behaviour, set REQUEST_STRUCTURED_OUTPUT_FROM_LLM to `false`." ) # Emit error handling event for structured output fallback yield build_stream_event_error_handling( is_start=True, error_type="IncorrectToolCall", error_message="Detected incorrect tool call, disabling structured output", will_retry=True, ) yield build_stream_event_error_handling(is_start=False) + # Close the parse_response span before retrying + yield build_stream_event_parse_response( + is_start=False, + tool_call_count=0, + finish_reason="retry_structured_output", + ) # disable structured output going forward and and retry sentry_helper.capture_structured_output_incorrect_tool_call() response_format = None max_steps = max_steps + 1 continue
🤖 Fix all issues with AI agents
In `@docs/otel-tracing.md`:
- Around line 85-103: Update the documentation examples so the LLM span names
use an Anthropic Claude model instead of "gpt-4": replace occurrences of the
span string "chat gpt-4" (seen in the example hierarchy and the "chat {model}"
naming examples) with the Anthropic model identifier (e.g., "chat
anthropic/claude-sonnet-4-5-20250929"), keeping the surrounding span formats
like "invoke_agent HolmesGPT" and "execute_tool {tool_name}" unchanged.
In `@experimental/ag-ui/server-agui.py`:
- Around line 578-589: The span attributes are using metric instrument constants
(otel_attr.METRIC_AGENT_TOOL_CALLS, otel_attr.METRIC_AGENT_ITERATIONS) instead
of the intended span attribute keys; update the calls to root_span.set_attribute
to use the attribute constants otel_attr.TOOL_CALL_COUNT and
otel_attr.AGENT_ITERATION respectively, leaving the other attributes
(INPUT_TOKENS, OUTPUT_TOKENS, TOTAL_TOKENS, COST_USD, RESULT_SUCCESS) unchanged
so all final metrics are recorded with the correct otel_attr keys.
- Around line 22-40: Unconditionally importing OpenTelemetry and calling
initialization causes OTEL to run even when disabled; move the OTEL-related
imports (get_tracer, set_span_error, experimental.otel.attributes as otel_attr,
and metrics functions like init_otel_metrics, record_token_usage,
record_operation_duration, record_tool_duration, increment_iterations,
increment_tool_calls, increment_errors) into a guarded try/except block and only
perform initialization calls (TracingFactory.init_otel() and
init_otel_metrics()) when OTEL is enabled (check OTEL_ENABLED == "true"); ensure
any failures fall back cleanly by catching ImportError/Exception and leaving
tracing/metrics references safe for the rest of the module (e.g., set
tracer-related names to None or no-op placeholders if imports fail).
In `@experimental/otel/attributes.py`:
- Around line 14-31: The import from
opentelemetry.semconv._incubating.gen_ai_attributes (e.g., GEN_AI_AGENT_NAME,
GEN_AI_REQUEST_MODEL, GenAiOperationNameValues) relies on an unstable incubating
API; either explicitly pin opentelemetry-semantic-conventions in pyproject.toml
to a specific safe version (e.g., opentelemetry-semantic-conventions = "0.X.Y")
to freeze the transitive API, or copy the needed constants locally into this
module and stop importing from _incubating; at minimum add a module docstring at
the top of experimental/otel/attributes.py stating the dependency on the
incubating namespace and that updates may be required when upstream changes
occur so reviewers know the risk.
In `@experimental/otel/otel_logging.py`:
- Around line 138-143: The current condition skips logging a legitimate zero
cost because it uses a truthy check; in otel_logging.py update the conditional
that appends cost to msg_parts to check for None explicitly (use "cost_usd is
not None" instead of "if cost_usd") so zero values are included, leaving the
existing formatting logic (f"cost=${cost_usd:.6f}") and other parts like
finish_reason/msg_parts unchanged.
In `@experimental/otel/tracing.py`:
- Around line 268-270: The double dict unpacking in the call
super().request(method, url, **kwargs, **send_kwargs) can raise TypeError if
keys overlap; change it to build a single kwargs dict and check/resolve
conflicts before unpacking. For example, create merged = kwargs.copy(), check
for intersection between merged and send_kwargs (raise or decide precedence),
then merged.update(send_kwargs) and call super().request(method, url, **merged)
so that overlapping keys are handled safely; reference the request method where
super().request is called in experimental/otel/tracing.py.
- Around line 14-16: The top-level boto3/botocore imports create an unnecessary
hard dependency; change to lazy imports by removing boto3 and botocore imports
from the module top and instead import them inside needs_aws_auth(), inside
_create_osis_session(), and inside the AWSSigV4Session implementation where they
are used; specifically import boto3, from botocore.auth import SigV4Auth and
from botocore.awsrequest import AWSRequest (or the specific symbols used) within
those functions/classes so environments that don't need AWS SigV4 auth won’t
require boto3/botocore installed.
- Around line 80-104: LoggingSpanExporter must subclass the SpanExporter base so
isinstance checks in BatchSpanProcessor succeed; change the class declaration to
inherit from SpanExporter (e.g. class LoggingSpanExporter(SpanExporter):) and
update the constructor type for wrapped_exporter from OTLPSpanExporter to
SpanExporter so the wrapper accepts any exporter implementing the base; keep the
existing export, shutdown and force_flush method implementations unchanged so
they satisfy the SpanExporter contract.
In `@holmes/core/tracing.py`:
- Around line 36-46: The fallback assigns truncate and set_span_error to None
which causes TypeError when OTELSpan.log and OTELSpan.__exit__ call them; fix by
providing safe no-op fallbacks or by guarding calls. Update the except
ImportError block to set truncate and set_span_error to small no-op functions
with the same signatures as the real implementations (so OTELSpan.log and
OTELSpan.__exit__ can call them safely), or alternatively add conditional checks
in OTELSpan.log and OTELSpan.__exit__ to only call truncate and set_span_error
when OTEL_EXPERIMENTAL_AVAILABLE (or when the function is not None); ensure
get_tracer/init_otel_tracer are similarly safe or guarded.
- Around line 523-538: TracingFactory.init_otel currently calls
init_otel_tracer() unconditionally which will raise TypeError when
init_otel_tracer is None; modify init_otel to first check the
OTEL_EXPERIMENTAL_AVAILABLE flag (and/or that init_otel_tracer is callable) and
if not available log a clear warning and return False, otherwise call
init_otel_tracer(), set TracingFactory._otel_initialized from the result, and
preserve the existing exception handling around the call.
- Around line 369-374: OTELTracer._ensure_initialized currently calls
init_otel_tracer() and get_tracer() unconditionally; add a defensive guard that
checks the OTEL availability (either OTEL_EXPERIMENTAL_AVAILABLE or that
init_otel_tracer and get_tracer are not None/callable) before calling them, and
if unavailable raise a clear RuntimeError (or return without initializing)
explaining OTEL is not installed/enabled; update OTELTracer._ensure_initialized
to validate init_otel_tracer and get_tracer before invocation and only set
_initialized and _native_tracer after successful calls.
- Line 11: The unconditional "from opentelemetry import trace as otel_trace"
makes OpenTelemetry a hard dependency; wrap that import in a try/except and set
a module-level flag (e.g., OTEL_API_AVAILABLE = True/False) so the import is
optional, then only define or instantiate OTEL-specific classes (OTELSpan,
OTELTracer) when OTEL_API_AVAILABLE is True; ensure DummySpan remains always
available for tool_calling_llm.py and update any code paths referencing
otel_trace/OTELTracer to check OTEL_API_AVAILABLE before using them.
🧹 Nitpick comments (14)
experimental/otel/otel_logging.py (2)
39-51: Consider extracting shared_get_otel_enabled()into a common location.
_get_otel_enabled()is identically defined inexperimental/otel/tracing.py(line 107),experimental/otel/metrics.py(line 30), and here (line 39). Similarly,_get_logs_enabledand_get_log_content_enabledare defined but never used in this file.
246-249: Use explicit!sconversion flag per Ruff RUF010.- f"Agent error: {agent_name} error_type={err_type} message={str(error)}", + f"Agent error: {agent_name} error_type={err_type} message={error!s}",server.py (2)
236-268: Tracer created per request; consider creating it once at module level.
TracingFactory.create_tracer("otel")is called inside the middleware on every request. This is unnecessary overhead — create the tracer once and reuse it.Also,
http.status_codeshould be an integer per OTEL HTTP semantic conventions, not a string.Proposed fix
+# Create tracer once at module level +_otel_tracer = TracingFactory.create_tracer("otel") if otel_enabled else None + if otel_enabled: `@app.middleware`("http") async def otel_tracing_middleware(request: Request, call_next): """Create OTEL spans for incoming API requests.""" if request.url.path in ("/healthz", "/readyz", "/docs", "/openapi.json"): return await call_next(request) - tracer = TracingFactory.create_tracer("otel") + tracer = _otel_tracer span_name = f"{request.method} {request.url.path}" with tracer.start_trace(span_name, SpanType.TASK) as span: span.set_attributes( span_attributes={ "http.method": request.method, "http.url": str(request.url), "http.route": request.url.path, } ) try: response = await call_next(request) span.set_attributes( - span_attributes={"http.status_code": str(response.status_code)} + span_attributes={"http.status_code": response.status_code} ) return response
66-90: Missing return type annotation oninit_otel().As per coding guidelines, type hints are required for Python files.
Proposed fix
-def init_otel(): +def init_otel() -> bool:docs/otel-tracing.md (1)
17-52: Add language specifiers to fenced code blocks.The architecture diagram code blocks (lines 17 and 85) lack language specifiers, flagged by markdownlint (MD040). Use
textorplaintextfor ASCII diagrams.-``` +```text ┌─────────────────────────────────────────────────────────────────┐experimental/ag-ui/server-agui.py (1)
619-627: Minor Ruff findings in exception cleanup block.Line 621: loop variable
keyis unused (Ruff B007) — rename to_key. Line 649: use{e!s}instead of{str(e)}(Ruff RUF010).Proposed fix
- for key, span in list(active_spans.items()): + for _key, span in list(active_spans.items()): try: set_span_error(span, e) span.end() - except Exception: - pass # Best effort cleanup + except Exception: # noqa: BLE001 + pass # Best effort span cleanup- message=f"Agent encountered an error: {str(e)}", + message=f"Agent encountered an error: {e!s}",experimental/otel/test_otel_integration.py (2)
1-28: Integration test is a standalone script rather than a pytest test.This test can't be discovered by
pytestand manipulates private module state (tracing._initialized,tracing._tracer_provider). The docstring notes it "can be converted to pytest later" — consider creating a tracking issue for that migration.
111-115: Resetting private module state is fragile.Directly mutating
tracing._initializedandtracing._tracer_providercouples tests to internal implementation details. If the module internals change, these tests break silently. This is acceptable for a quick verification script but should be addressed if migrated to pytest (e.g., via a properreset()function or fixture).experimental/otel/test_otel.py (1)
571-617: File-content string matching tests are brittle and will break on any refactor.
test_server_uses_tracing_factoryandtest_agui_uses_tracing_factoryopen source files and assert on exact string patterns like'TracingFactory.create_tracer("otel")'. These break on any formatting change, import aliasing, or code reorganization — without indicating a real regression. Consider replacing with import-based tests that verify the actual runtime behavior instead.experimental/otel/tracing.py (1)
345-444:_initialized = Trueon failure prevents any recovery.Lines 370–376 and 443 set
_initialized = Trueeven when initialization fails or OTEL is disabled. This means if env vars are corrected after a failed first attempt, a restart is required. The warning comment at line 288-291 acknowledges thread-safety concerns but not this "no retry" behavior — consider documenting it in the function docstring.holmes/core/tool_calling_llm.py (2)
1154-1178:duration_msis hardcoded to0, making the metric meaningless.The
TOOL_INVOKE_ENDevent always reportsduration_ms=0. The start event is emitted before the executor.submit(), and the end event is emitted afterfuture.result(), so the timing information is actually available — you'd just need to capturetime.monotonic()around the tool invocation.Also,
APPROVAL_REQUIREDstatus is mapped to"FAILURE", which could be misleading in dashboards. Consider a distinct status value.♻️ Sketch: capture actual duration and distinguish approval status
+import time + # ... inside the for loop over futures ... + # Track tool start time (or capture per-future before submit) # Emit tool invoke end event for OTEL tracing tool_status = ( "SUCCESS" if tool_call_result.result.status == StructuredToolResultStatus.SUCCESS - else "FAILURE" + elif tool_call_result.result.status + == StructuredToolResultStatus.APPROVAL_REQUIRED + else "APPROVAL_REQUIRED" + else "FAILURE" )For duration, consider recording
time.monotonic()beforeexecutor.submit()per-future, then computing elapsed in the completion loop.
1024-1031: Error events are emitted before determining the error type.Both
error_handling_startanderror_handling_endevents are emitted at Lines 1025–1031 before theifon Line 1032 checks whether this is the specific Azure error or a generic re-raise. Thewill_retry=Falseis set unconditionally, but only the Azure-specific case gets a custom message; the generic re-raise may benefit from different handling. This is a minor concern since both paths ultimately raise, but it slightly reduces the diagnostic value of the events.experimental/otel/metrics.py (1)
76-99: Imports inside function body violate coding guidelines.The coding guidelines state: "ALWAYS place Python imports at the top of the file, not inside functions or methods." While the dynamic import pattern is pragmatic for optional dependencies, it conflicts with the explicit guideline.
Additionally,
_create_osis_session(Line 85) is a private function being used across module boundaries. Consider exposing it as a public function or creating a shared utility.As per coding guidelines:
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods.♻️ Move imports to module top and make cross-module function public
Move the conditional imports to the top of the file with a try/except guard:
+try: + from opentelemetry.exporter.otlp.proto.http.metric_exporter import ( + OTLPMetricExporter, + ) + from experimental.otel.tracing import needs_aws_auth, create_osis_session +except ImportError: + OTLPMetricExporter = None # type: ignore + needs_aws_auth = None # type: ignore + create_osis_session = None # type: ignoreAnd in
experimental/otel/tracing.py, rename_create_osis_session→create_osis_sessionto reflect its cross-module usage.holmes/utils/stream.py (1)
237-241: Magic number8192duplicated — consider using a shared constant.The truncation limit
8192appears inbuild_stream_event_tool_invoke_start(Line 240),build_stream_event_tool_invoke_end(Line 273), and matchesMAX_ATTRIBUTE_SIZEinexperimental/otel/attributes.py. Using a shared constant would prevent drift if the limit changes.Also applies to: 272-273
| from opentelemetry.semconv._incubating.attributes.gen_ai_attributes import ( | ||
| GEN_AI_AGENT_NAME, | ||
| GEN_AI_CONVERSATION_ID, | ||
| GEN_AI_OPERATION_NAME, | ||
| GEN_AI_PROVIDER_NAME, | ||
| GEN_AI_REQUEST_MAX_TOKENS, | ||
| GEN_AI_REQUEST_MODEL, | ||
| GEN_AI_REQUEST_TEMPERATURE, | ||
| GEN_AI_RESPONSE_FINISH_REASONS, | ||
| GEN_AI_RESPONSE_MODEL, | ||
| GEN_AI_SYSTEM, | ||
| GEN_AI_TOKEN_TYPE, | ||
| GEN_AI_TOOL_CALL_ID, | ||
| GEN_AI_TOOL_NAME, | ||
| GEN_AI_USAGE_INPUT_TOKENS, | ||
| GEN_AI_USAGE_OUTPUT_TOKENS, | ||
| GenAiOperationNameValues, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
opentelemetry-semantic-conventions incubating namespace stability
💡 Result:
In OpenTelemetry Semantic Conventions, “incubating” denotes unstable/preview conventions that do not carry stability guarantees and may change (including breaking changes) without a major-version bump.
- JavaScript package behavior:
@opentelemetry/semantic-conventionsexposes only stable conventions and follows SemVer (no breaking changes except major versions). The separate entry point@opentelemetry/semantic-conventions/incubatingcontains unstable (“experimental”) conventions and may include breaking changes in minor releases. Instrumentations are advised not to rely on incubating exports at runtime (copy constants locally or otherwise protect against churn). [1] - Generated semconv artifacts / namespaces: the spec’s code-generation guidance explicitly separates artifacts and namespaces:
- stable:
opentelemetry-semconvwith namespaceopentelemetry.semconv - incubating:
opentelemetry-semconv-incubatingwith namespaceopentelemetry.semconv.incubating(containing stable + unstable) [2]
- stable:
- Practical implication: anything under an incubating namespace/entry point should be treated as non-stable, meaning attribute/metric/event names and group definitions can be revised as part of the stabilization process. (The spec separately defines “group stability” levels like
development/alpha/beta/.../stableand emphasizes that stable instrumentations shouldn’t emit unstable parts by default.) [3]
Sources:
[1] OpenTelemetry JS semconv docs (incubating entry point + stability/SemVer notes) (open-telemetry.github.io)
[2] OpenTelemetry semconv code-generation guidance (artifact + namespace names) (opentelemetry.io)
[3] OpenTelemetry semantic convention groups spec (stability levels + rules) (opentelemetry.io)
Citations:
- 1: https://open-telemetry.github.io/opentelemetry-js/modules/_opentelemetry_semantic-conventions.html?utm_source=openai
- 2: https://opentelemetry.io/docs/specs/semconv/non-normative/code-generation/?utm_source=openai
- 3: https://opentelemetry.io/docs/specs/semconv/general/semantic-convention-groups/?utm_source=openai
🏁 Script executed:
# Find and examine the project's pyproject.toml or requirements files
find . -name "pyproject.toml" -o -name "requirements*.txt" -o -name "setup.py" | head -20Repository: HolmesGPT/holmesgpt
Length of output: 662
🏁 Script executed:
# Check the current experimental/otel/attributes.py file structure
head -35 experimental/otel/attributes.pyRepository: HolmesGPT/holmesgpt
Length of output: 1203
🏁 Script executed:
# Examine pyproject.toml for opentelemetry-semantic-conventions version constraint
cat pyproject.toml | grep -A 5 -B 5 "opentelemetry-semantic-conventions"Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Check if there are any comments or documentation about this incubating dependency
rg -i "incubating|unstable|semconv" experimental/otel/ -A 2 -B 2Repository: HolmesGPT/holmesgpt
Length of output: 1037
🏁 Script executed:
# Read the full pyproject.toml to see all dependencies
cat pyproject.toml | head -100Repository: HolmesGPT/holmesgpt
Length of output: 2532
🏁 Script executed:
# Search for opentelemetry in all dependency files
rg "opentelemetry" pyproject.tomlRepository: HolmesGPT/holmesgpt
Length of output: 173
Pin opentelemetry-semantic-conventions version and document the incubating dependency risk.
The opentelemetry.semconv._incubating namespace is not covered by stability guarantees and can introduce breaking changes in minor version releases (not just major versions). Currently, opentelemetry-semantic-conventions is not pinned explicitly in pyproject.toml (it's a transitive dependency via other OpenTelemetry packages). Either:
- Add an explicit, tightly pinned version constraint (e.g.,
opentelemetry-semantic-conventions = "0.X.Y") to guard against unexpected API changes, or - Copy the
gen_ai_attributesconstants locally to insulate this code from upstream churn, or - Add clear documentation in the module docstring noting that this code depends on an unstable API and may require updates when the semconv spec stabilizes.
🤖 Prompt for AI Agents
In `@experimental/otel/attributes.py` around lines 14 - 31, The import from
opentelemetry.semconv._incubating.gen_ai_attributes (e.g., GEN_AI_AGENT_NAME,
GEN_AI_REQUEST_MODEL, GenAiOperationNameValues) relies on an unstable incubating
API; either explicitly pin opentelemetry-semantic-conventions in pyproject.toml
to a specific safe version (e.g., opentelemetry-semantic-conventions = "0.X.Y")
to freeze the transitive API, or copy the needed constants locally into this
module and stop importing from _incubating; at minimum add a module docstring at
the top of experimental/otel/attributes.py stating the dependency on the
incubating namespace and that updates may be required when upstream changes
occur so reviewers know the risk.
| ### Environment Variables | ||
|
|
||
| | Variable | Description | Default | | ||
| |----------|-------------|---------| | ||
| | `OTEL_ENABLED` | Enable OTEL tracing (`true`/`false`) | `false` | | ||
| | `OTEL_EXPORTER_OTLP_ENDPOINT` | OTLP endpoint URL | Required | | ||
| | `OTEL_AWS_PROFILE` | AWS profile for OSIS authentication | None | | ||
| | `OTEL_AWS_REGION` | AWS region for OSIS (auto-detected from endpoint) | Auto | | ||
| | `OTEL_METRICS_ENABLED` | Enable OTEL metrics | `true` | | ||
| | `OTEL_DEBUG` | Enable debug logging for span lifecycle | `false` | | ||
|
|
There was a problem hiding this comment.
Make sure these are aligned with OpenTelemetry official environment variables:
https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/
| ### Environment Variables | |
| | Variable | Description | Default | | |
| |----------|-------------|---------| | |
| | `OTEL_ENABLED` | Enable OTEL tracing (`true`/`false`) | `false` | | |
| | `OTEL_EXPORTER_OTLP_ENDPOINT` | OTLP endpoint URL | Required | | |
| | `OTEL_AWS_PROFILE` | AWS profile for OSIS authentication | None | | |
| | `OTEL_AWS_REGION` | AWS region for OSIS (auto-detected from endpoint) | Auto | | |
| | `OTEL_METRICS_ENABLED` | Enable OTEL metrics | `true` | | |
| | `OTEL_DEBUG` | Enable debug logging for span lifecycle | `false` | | |
| ### Environment Variables | |
| | Variable | Description | Default | | |
| |----------|-------------|---------| | |
| | `OTEL_SDK_DISABLED` | Disable OTEL tracing (`true`/`false`) | `true` | | |
| | `OTEL_EXPORTER_OTLP_ENDPOINT` | OTLP endpoint URL | Required | | |
| | `OTEL_RESOURCE_ATTRIBUTES` | Key-value pairs to be used as resource attributes, eg AWS profile and region for OSIS | None | | |
| | `OTEL_METRICS_EXPORTER` | Metrics exporter to be used, set to `none` to disable metrics | `true` | | |
| | `OTEL_LOG_LEVEL` | Set to `debug` to enable debug logging for span lifecycle | `info` | |
Note, that we (OpenTelemetry) are currently working on a configuration file that enables much richer and broader options.
| # AWS OSIS (OpenSearch Ingestion Service) | ||
| export OTEL_ENABLED=true | ||
| export OTEL_EXPORTER_OTLP_ENDPOINT=https://pipeline-id.us-east-1.osis.amazonaws.com/otel-trace/v1/traces | ||
| export OTEL_AWS_PROFILE=your-aws-profile | ||
| export OTEL_AWS_REGION=us-east-1 | ||
| ``` |
There was a problem hiding this comment.
Not sure if those OTEL_AWS are setting resource attributes as I have assumed above or are something specific to OSIS, if this is the case I would recommend to name them AWS_OSIS_... or OSIS_AWS_... but not put them under OTEL_
aabfcf4 to
4f517ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (11)
tests/test_bash_session_prefix_flow.py (1)
43-49: LGTM — minor:mock_response.usage = MagicMock()reassignment is redundantThe
finish_reasonternary is correct ("stop"/"tool_calls"matches the OpenAI spec), and the token counts are arithmetically consistent. The hardcoded values are appropriate for a test fixture.One small nit: since
mock_responseis already aMagicMock(),mock_response.usageis already an auto-generatedMagicMock()child. Reassigning it to a newMagicMock()on line 46 before setting attributes on lines 47–49 has no net effect and can be dropped.🧹 Proposed simplification
- # Set usage to avoid MagicMock serialization issues - mock_response.usage = MagicMock() - mock_response.usage.prompt_tokens = 100 - mock_response.usage.completion_tokens = 50 - mock_response.usage.total_tokens = 150 + # Set usage fields explicitly to avoid MagicMock serialization issues in OTEL tracing + mock_response.usage.prompt_tokens = 100 + mock_response.usage.completion_tokens = 50 + mock_response.usage.total_tokens = 150🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_bash_session_prefix_flow.py` around lines 43 - 49, Remove the redundant reassignment of mock_response.usage to a new MagicMock: since mock_response is already a MagicMock, its .usage child is auto-created, so delete the line "mock_response.usage = MagicMock()" and leave the subsequent assignments (mock_response.usage.prompt_tokens, .completion_tokens, .total_tokens) as-is so the test still sets the token counts while avoiding an unnecessary override of the existing mock object.holmes/config.py (1)
191-246: Alignload_from_fileto use Pydantic v2'smodel_dump()for consistency.
load_from_envnow correctly uses Pydantic v2'smodel_dump()(line 244), while the adjacentload_from_filemethod still calls the deprecated.dict()(line 182). Both methods share the same "dump → merge → reconstruct" pattern, so they should use the same API. In Pydantic v2,.dict()is deprecated and emits a deprecation warning; it now simply delegates tomodel_dump().♻️ Align `load_from_file` to use `model_dump()`
- merged_config = config_from_file.dict() + merged_config = config_from_file.model_dump()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/config.py` around lines 191 - 246, The load_from_file method currently uses the deprecated Pydantic v2 .dict() call when reconstructing the Config instance; update load_from_file to call model_dump() instead (mirror the pattern used in load_from_env), so replace the .dict() usage in load_from_file with model_dump() when creating merged_config and then pass that to Config(...) or cls(...); reference the load_from_file function and the merged_config variable to locate the change.experimental/otel/otel_logging.py (2)
249-252: Use f-string conversion flag instead of explicitstr()call.Ruff RUF010 flags
str(error)inside f-strings. Use the!sconversion flag for clarity and consistency.Proposed fix
err_type = error_type or type(error).__name__ logger.error( - f"Agent error: {agent_name} error_type={err_type} message={str(error)}", + f"Agent error: {agent_name} error_type={err_type} message={error!s}", exc_info=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/otel/otel_logging.py` around lines 249 - 252, Replace the explicit str() call inside the f-string in the logger.error call so it uses the f-string conversion flag; in the block where err_type is set (err_type = error_type or type(error).__name__) and logger.error is invoked, change the message to use {error!s} (e.g., "Agent error: {agent_name} error_type={err_type} message={error!s}") and keep exc_info=True unchanged so Ruff RUF010 is satisfied and formatting remains consistent.
39-54: Remove unused private helper functions_get_logs_enabledand_get_log_content_enabled.These functions are defined but never called anywhere in the codebase. Since they are private (underscore-prefixed), they're not part of the public API. Remove them or document them as placeholders for future use if intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/otel/otel_logging.py` around lines 39 - 54, Remove the two unused private helper functions _get_logs_enabled and _get_log_content_enabled from experimental/otel/otel_logging.py (they are defined but never called); if you intended to keep them as placeholders instead, add a short docstring comment above each explaining they are intentionally unused and add a # noqa or explicit usage comment to silence linters—otherwise simply delete the function definitions and any related unused imports or references.holmes/utils/stream.py (2)
237-241: Hardcoded truncation limit8192duplicatesMAX_ATTRIBUTE_SIZEfromattributes.py.The magic number
8192appears in multiple places (lines 240, 273) and is also defined asMAX_ATTRIBUTE_SIZEinexperimental/otel/attributes.py. While importing from the experimental module might not be desirable here (to avoid coupling core utils to experimental code), consider defining a local constant at the top of this file.Proposed fix
+# Maximum size for string payloads in stream events (matches OTEL attribute limits) +_MAX_PAYLOAD_SIZE = 8192 + + def build_stream_event_tool_invoke_start( tool_name: str, tool_call_id: str, tool_arguments: Optional[str] = None, ) -> StreamMessage: ... if tool_arguments is not None: - data["tool_arguments"] = ( - tool_arguments[:8192] if len(tool_arguments) > 8192 else tool_arguments - ) + data["tool_arguments"] = tool_arguments[:_MAX_PAYLOAD_SIZE] if len(tool_arguments) > _MAX_PAYLOAD_SIZE else tool_argumentsAlso applies to: 272-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/utils/stream.py` around lines 237 - 241, The hardcoded truncation value 8192 is duplicated and should be replaced by a named constant; add a local constant (e.g., MAX_ATTR_SIZE or LOCAL_MAX_ATTRIBUTE_SIZE) near the top of holmes/utils/stream.py and use it wherever tool_arguments (data["tool_arguments"]) is truncated (and the similar truncation at lines handling other attributes around 272-273) so the truncation uses that constant instead of the magic number; update occurrences of the literal 8192 to reference the new constant (keep the same numeric value).
325-325: Minor inconsistency:data = {}lacks type annotation while similar variables on lines 299, 355 are annotated asdict[str, Any].For consistency across the builder functions:
Proposed fix
- data = {} + data: dict[str, Any] = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/utils/stream.py` at line 325, The variable "data" in holmes/utils/stream.py is declared as data = {} without a type annotation while similar builder variables use dict[str, Any]; update the declaration of data (the local variable in the builder function where data is set) to include an explicit type hint (data: dict[str, Any] = {}) so it matches the other annotated variables (see the other builder variables declared as dict[str, Any]) and keeps typing consistent across the function(s) using data.holmes/core/tracing.py (2)
343-359:OTELSpan.set_attributesconvertsNonevalues to empty strings, which may mask missing data.Line 355 sets
Noneattribute values as empty strings (""). In OTEL, it's generally better to omit attributes entirely rather than set them to empty strings, as this can confuse downstream queries (distinguishing "not set" from "explicitly empty").Proposed fix
if span_attributes: for key, value in span_attributes.items(): - if value is None: - self._span.set_attribute(key, "") - elif isinstance(value, (str, int, float, bool)): + if value is None: + continue # Omit unset attributes + elif isinstance(value, (str, int, float, bool)): self._span.set_attribute(key, value) else: self._span.set_attribute(key, str(value))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tracing.py` around lines 343 - 359, OTELSpan.set_attributes currently turns None values into empty strings which can mask missing attributes; update the method (set_attributes) so that when iterating span_attributes it simply skips any key whose value is None instead of calling self._span.set_attribute(key, ""); keep the existing handling for str/int/float/bool and fallback to str(...) for other types, and leave the name/type handling unchanged (i.e., still call self._span.set_attribute("span.name", name) when name is provided).
326-341:OTELSpan.log— metadata values are always stringified, losing type information.Line 341 converts all metadata values to
str(value). OTEL attributes supportint,float,bool, andstrnatively. Preserving native types would improve query filtering in observability backends.Proposed fix
if metadata: for key, value in metadata.items(): - self._span.set_attribute(f"metadata.{key}", str(value)) + if isinstance(value, (str, int, float, bool)): + self._span.set_attribute(f"metadata.{key}", value) + else: + self._span.set_attribute(f"metadata.{key}", str(value))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tracing.py` around lines 326 - 341, OTELSpan.log currently stringifies all metadata values; change the metadata loop in the log method to preserve OTEL-native types by passing ints, floats, and bools directly to self._span.set_attribute (and strings after applying the existing _truncate). For any non-primitive types (e.g., dicts, objects), fall back to str(value) before setting the attribute. Keep using the same _span.set_attribute call and the existing _truncate behavior only for string values so that numeric and boolean metadata remain typed for better querying.experimental/otel/metrics.py (2)
31-36:_get_otel_enabledis duplicated across multiple modules.This same function appears in
otel_logging.py(line 39) and here (line 31), with identical logic. Consider extracting it to a shared utility (e.g., inattributes.pyor a newconfig.py) to keep it DRY.Also applies to: 39-44, 47-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/otel/metrics.py` around lines 31 - 36, Extract the duplicated _get_otel_enabled function into a single shared helper (e.g., create or use attributes.py or a new config.py) and have both experimental/otel/metrics.py and experimental/otel/otel_logging.py import and call that single helper instead of defining their own copies; remove the local _get_otel_enabled definitions in metrics.py and otel_logging.py, add a clear function name (e.g., _get_otel_enabled) exported from the shared module, and update the imports/usages in both modules to reference the shared symbol.
61-66: Multipleglobalstatements for mutable module-level state.This pattern works but is fragile under concurrent initialization. Since
init_otel_metricsis expected to be called once at startup, this is acceptable, but consider adding a brief comment noting the single-initialization assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/otel/metrics.py` around lines 61 - 66, Add a brief comment above the module-level globals stating that these variables (_meter, _meter_provider, _token_usage_histogram, _operation_duration_histogram, _tool_duration_histogram, _agent_iterations_counter, _tool_calls_counter, _error_counter) are intended to be initialized once at startup by init_otel_metrics and are not thread-safe; this documents the single-initialization assumption and concurrency fragility so future readers know why multiple global statements are used and that init_otel_metrics must be called once before concurrent use.experimental/otel/__init__.py (1)
1-100: Eager imports of all submodules at package load time.Importing all four submodules (
attributes,metrics,otel_logging,tracing) at package load means anyfrom experimental.otel import Xtriggers loading every submodule. Since heavy dependencies (OTLP exporters, AWS sessions) are deferred to function calls within those modules, this is acceptable for now. If startup cost becomes a concern, consider lazy imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/otel/__init__.py` around lines 1 - 100, The package currently eagerly imports everything from experimental.otel.attributes, metrics, otel_logging, and tracing on module load (symbols like AGENT_ITERATION, get_meter, OTELContextFormatter, get_tracer, etc.), causing unnecessary startup work; change __init__.py to use lazy imports instead: remove the top-level from ... import ... lines and implement a module-level lazy loader (e.g., __getattr__ and __dir__ or importlib.import_module) that imports the appropriate submodule on first access and returns the requested symbol (map attribute names such as AGENT_ITERATION, increment_errors, OTELContextFormatter, get_tracer, init_otel_tracer, etc., to their source submodules), and keep a defined __all__ listing the exported names so external users can still do from experimental.otel import get_meter or AGENT_ITERATION without triggering all submodules upfront.
🤖 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/otel-tracing.md`:
- Around line 256-278: Consolidate the small "Debug Logging" subsection into the
main "Troubleshooting" block: remove the separate "Debug Logging" header and
replace it with bold inline text (e.g., **Debug logging**) and merge all
troubleshooting commands into a single annotated code block containing the
environment and debug commands (e.g., setting OTEL_SDK_DISABLED=false, checking
HOLMES_AWS_OSIS_PROFILE, note about payload truncation, and the LOG_LEVEL=DEBUG
python server.py command) with short comments for each line; edit the sections
referencing the quoted issues ("OTEL tracing requested but OTEL_SDK_DISABLED is
not set to 'false'", "Failed to create OSIS session", "payload too large") and
the existing LOG_LEVEL example so they appear as explanatory lines surrounding
one unified fenced code block instead of separate headers.
- Around line 17-52: The fenced ASCII diagram blocks (the triple-backtick blocks
containing the "Entry Points / TracingFactory / OTLP Exporter" diagram and the
span hierarchy fences) are missing language identifiers and trigger MD040;
update each opening fence from ``` to ```text (or ```plaintext) so the diagram
is treated as plain text; ensure you update the main ASCII diagram block and the
other affected fenced blocks (the second occurrence referenced in the comment)
so both render warnings are resolved.
In `@experimental/ag-ui/server-agui.py`:
- Around line 72-86: The NameError occurs because _NoopSpan and _NoopTracer are
only defined inside the ImportError fallback, but when _otel_available is True
and OTEL_SDK_DISABLED is set the code references _NoopTracer from the else
branch and crashes; fix by defining the no-op classes (_NoopSpan and
_NoopTracer) unconditionally (move their definitions out of the ImportError
branch to module scope) or ensure both branches import/define them before use so
the code paths that check _otel_available and OTEL_SDK_DISABLED can safely
instantiate _NoopTracer; apply the same change for the duplicate definitions
referenced later around the other instrumentation block (the second
_NoopSpan/_NoopTracer usage).
In `@experimental/otel/attributes.py`:
- Around line 158-178: The truncate function can return a string longer than
max_size when max_size is smaller than len(TRUNCATION_MARKER); update truncate
(referencing truncate, TRUNCATION_MARKER, MAX_ATTRIBUTE_SIZE) to first handle
non-positive max_size by returning "" and then handle the small-max case by
returning at most the first max_size characters of TRUNCATION_MARKER when
max_size <= len(TRUNCATION_MARKER); otherwise keep the existing logic but ensure
the sliced prefix length is max_size - len(TRUNCATION_MARKER) so the final
string never exceeds max_size.
In `@experimental/otel/test_otel_integration.py`:
- Around line 107-121: In test_tracer_initialization and any helper functions in
this test module that currently import modules inside function bodies, move
those imports to module scope at the top of
experimental/otel/test_otel_integration.py and remove inline imports; also add
explicit return type hints (e.g., -> bool, -> int or -> None) to every helper
function and test helper used in this file so signatures satisfy the project's
mypy rules; ensure the test still resets tracing._initialized and
tracing._tracer_provider and continues to call init_otel_tracer() unchanged.
In `@experimental/otel/test_otel.py`:
- Around line 19-27: The test function test_truncate_function imports
MAX_ATTRIBUTE_SIZE and truncate inside the function and lacks a return type;
move the import "from experimental.otel.attributes import MAX_ATTRIBUTE_SIZE,
truncate" to the module top alongside other imports, and add an explicit return
annotation "-> None" to test_truncate_function (and any other test helpers) so
mypy/type-checking passes; keep behavior unchanged and ensure only import and
signature changes touch the function.
- Around line 252-360: Tests and examples use "gpt-4" model IDs; change them to
the Claude model ID (e.g., "claude-4.5"). Update all occurrences of "gpt-4" in
this file including calls like record_token_usage, record_operation_duration,
increment_iterations, increment_tool_calls, log_llm_call,
build_stream_event_llm_iteration_start,
build_stream_event_llm_iteration_complete, and any assertion comparing
event.data["model"] or similar to use "claude-4.5" so tests and examples comply
with the Anthropic Claude requirement.
In `@holmes/core/tool_calling_llm.py`:
- Around line 953-965: The inline comments around the context-check events are
restating the code; change them to explain the rationale: clarify that the calls
to build_stream_event_context_check before and after limit_input_context_window
are used to create OTEL trace spans/events that correlate context-trimming
operations (limit_input_context_window) with the surrounding stream processing
for observability and debugging; update the two comments adjacent to
build_stream_event_context_check and the limit_input_context_window call to
mention intent (e.g., "start/end OTEL event to correlate context trimming with
trace spans for visibility into message windowing performed by
limit_input_context_window") so future readers understand why the events are
emitted.
In `@holmes/core/tracing.py`:
- Around line 466-499: CompositeSpan.start_span currently passes span_type
positionally which breaks Braintrust's Span.start_span expecting a named
parameter type (string); change the delegation to call start_span with explicit
keywords (e.g. s.start_span(name=name, span_type=span_type, **kwargs)) and
handle Braintrust spans by detecting that span object (or by duck-typing on the
start_span signature) and passing type=<mapped_string> where <mapped_string>
converts your SpanType enum to one of Braintrust's allowed strings
("llm","score","function","eval","task","tool"); update CompositeSpan.start_span
to branch per span kind (OTELSpan/DummySpan use span_type=..., Braintrust Span
use type=<mapped_string>) so calls match each implementation's parameter names
and types.
In `@server.py`:
- Around line 235-242: Replace the current docstring for otel_tracing_middleware
(and/or the inline comment above the middleware) with a concise "why"
explanation: state that when otel_enabled and a tracer is created via
TracingFactory.create_tracer("otel"), this middleware establishes root
OpenTelemetry spans for incoming requests so request latency can be correlated
with downstream spans and traces are linked across services (or remove the
comment if you prefer no explanation). Keep the wording short and focused on
intent (correlation and distributed tracing), and reference
otel_tracing_middleware and TracingFactory.create_tracer("otel") so reviewers
can find it easily.
- Around line 63-91: The function init_otel lacks a return type annotation and
performs a local import; hoist the optional import by adding a module-level
try/except ImportError that sets a module symbol (e.g., init_otel_metrics =
None) so the dependency remains optional, then update init_otel to declare a
return type (-> bool) and use the hoisted init_otel_metrics symbol (check for
None before calling) to determine metrics_ok; ensure the function always returns
a boolean (True if tracing_ok or metrics_ok, False otherwise) and preserves the
existing logging behavior.
In `@tests/test_otel_tracing.py`:
- Around line 193-206: In TestOTELAttributes, hoist per-test imports (e.g., from
experimental.otel.attributes import truncate, MAX_ATTRIBUTE_SIZE) out of each
test function to the module top and add explicit return type annotations to each
test method (e.g., def test_truncate_none_returns_empty(self) -> None, def
test_truncate_short_string_unchanged(self) -> None, def
test_truncate_exact_limit_unchanged(self) -> None) so mypy sees top-level
imports and typed test methods; update all test methods in this class that
import truncate or MAX_ATTRIBUTE_SIZE inside the body accordingly.
---
Duplicate comments:
In `@experimental/otel/attributes.py`:
- Around line 14-31: The code imports unstable symbols from
opentelemetry.semconv._incubating (e.g., GEN_AI_AGENT_NAME,
GEN_AI_CONVERSATION_ID, GEN_AI_OPERATION_NAME, GEN_AI_PROVIDER_NAME,
GEN_AI_REQUEST_MAX_TOKENS, GEN_AI_REQUEST_MODEL, GEN_AI_REQUEST_TEMPERATURE,
GEN_AI_RESPONSE_FINISH_REASONS, GEN_AI_RESPONSE_MODEL, GEN_AI_SYSTEM,
GEN_AI_TOKEN_TYPE, GEN_AI_TOOL_CALL_ID, GEN_AI_TOOL_NAME,
GEN_AI_USAGE_INPUT_TOKENS, GEN_AI_USAGE_OUTPUT_TOKENS, GenAiOperationNameValues)
which risks breakage; fix by either (A) copying these constant definitions and
GenAiOperationNameValues into this module (experimental/otel/attributes.py) and
replacing the imports with local definitions, or (B) enforce a pinned
opentelemetry version in our dependency config and add a comment referencing the
pinned version; choose (A) if you want stability without pinning, ensure the
copied values match upstream exactly and keep a comment noting origin and
upstream version.
In `@experimental/otel/metrics.py`:
- Around line 86-101: The code currently uses endpoint.replace("/v1/traces",
"/v1/metrics") (and directly imports the private _create_osis_session) which can
leave the metrics exporter pointed at the wrong path and couples metrics to a
private tracing implementation; change the logic in the block that calls
needs_aws_auth(endpoint) to (1) compute the metrics endpoint robustly by
parsing/normalizing the URL and ensuring the path ends with /v1/metrics (e.g.,
using urljoin/urllib.parse to replace or append the path instead of str.replace
so a bare base URL gets the correct /v1/metrics path and detect if replacement
was a no-op and log a warning), and (2) stop importing the private
_create_osis_session from experimental.otel.tracing — use a shared, public
factory (e.g., move session creation into a common helper like an exported
create_osis_session in a new/shared module and call that) so OTLPMetricExporter
is created with a validated metrics_endpoint and a non-private session creator.
In `@experimental/otel/otel_logging.py`:
- Around line 113-147: The log_llm_call function's previous falsy-check bug for
cost_usd needs to be fixed by checking explicitly for None; update the cost_usd
conditional in log_llm_call to use "if cost_usd is not None:" so 0.0 is logged,
ensure the formatted cost string uses "{cost_usd:.6f}", and keep the existing
logger.info(" ".join(msg_parts)) behavior (symbols: log_llm_call, cost_usd,
logger.info, msg_parts, iteration).
In `@holmes/core/tracing.py`:
- Around line 547-567: The change correctly guards TracingFactory.init_otel
against missing OTEL experimental modules by checking
OTEL_EXPERIMENTAL_AVAILABLE before calling init_otel_tracer; leave the
OTEL_EXPERIMENTAL_AVAILABLE check in place, ensure the method continues to set
TracingFactory._otel_initialized from the init_otel_tracer result, and keep the
try/except that logs failures around init_otel_tracer to avoid a TypeError when
the experimental module is None (references: TracingFactory.init_otel,
OTEL_EXPERIMENTAL_AVAILABLE, init_otel_tracer,
TracingFactory._otel_initialized).
- Around line 11-17: Apply the same guarded import pattern used for
opentelemetry (the otel_trace import and OTEL_API_AVAILABLE flag) to the
experimental module imports in this file (the imports around lines 42-52): wrap
each optional import in try/except ImportError, set a corresponding availability
boolean (e.g., <MODULE_NAME>_AVAILABLE) and assign the module variable to None
with a type: ignore fallback on ImportError, and ensure any downstream checks
use those availability flags before referencing the modules.
- Around line 387-398: Ensure _ensure_initialized sets self._initialized = True
when OTEL_EXPERIMENTAL_AVAILABLE is False and does not set self._native_tracer
so start_trace can fall back to DummySpan; keep the current guard logic in
_ensure_initialized, call init_otel_tracer() and set self._native_tracer =
get_tracer(self._service_name) only when OTEL_EXPERIMENTAL_AVAILABLE is True,
and verify start_trace inspects self._native_tracer before using it (so
start_trace/DummySpan fallback works).
---
Nitpick comments:
In `@experimental/otel/__init__.py`:
- Around line 1-100: The package currently eagerly imports everything from
experimental.otel.attributes, metrics, otel_logging, and tracing on module load
(symbols like AGENT_ITERATION, get_meter, OTELContextFormatter, get_tracer,
etc.), causing unnecessary startup work; change __init__.py to use lazy imports
instead: remove the top-level from ... import ... lines and implement a
module-level lazy loader (e.g., __getattr__ and __dir__ or
importlib.import_module) that imports the appropriate submodule on first access
and returns the requested symbol (map attribute names such as AGENT_ITERATION,
increment_errors, OTELContextFormatter, get_tracer, init_otel_tracer, etc., to
their source submodules), and keep a defined __all__ listing the exported names
so external users can still do from experimental.otel import get_meter or
AGENT_ITERATION without triggering all submodules upfront.
In `@experimental/otel/metrics.py`:
- Around line 31-36: Extract the duplicated _get_otel_enabled function into a
single shared helper (e.g., create or use attributes.py or a new config.py) and
have both experimental/otel/metrics.py and experimental/otel/otel_logging.py
import and call that single helper instead of defining their own copies; remove
the local _get_otel_enabled definitions in metrics.py and otel_logging.py, add a
clear function name (e.g., _get_otel_enabled) exported from the shared module,
and update the imports/usages in both modules to reference the shared symbol.
- Around line 61-66: Add a brief comment above the module-level globals stating
that these variables (_meter, _meter_provider, _token_usage_histogram,
_operation_duration_histogram, _tool_duration_histogram,
_agent_iterations_counter, _tool_calls_counter, _error_counter) are intended to
be initialized once at startup by init_otel_metrics and are not thread-safe;
this documents the single-initialization assumption and concurrency fragility so
future readers know why multiple global statements are used and that
init_otel_metrics must be called once before concurrent use.
In `@experimental/otel/otel_logging.py`:
- Around line 249-252: Replace the explicit str() call inside the f-string in
the logger.error call so it uses the f-string conversion flag; in the block
where err_type is set (err_type = error_type or type(error).__name__) and
logger.error is invoked, change the message to use {error!s} (e.g., "Agent
error: {agent_name} error_type={err_type} message={error!s}") and keep
exc_info=True unchanged so Ruff RUF010 is satisfied and formatting remains
consistent.
- Around line 39-54: Remove the two unused private helper functions
_get_logs_enabled and _get_log_content_enabled from
experimental/otel/otel_logging.py (they are defined but never called); if you
intended to keep them as placeholders instead, add a short docstring comment
above each explaining they are intentionally unused and add a # noqa or explicit
usage comment to silence linters—otherwise simply delete the function
definitions and any related unused imports or references.
In `@holmes/config.py`:
- Around line 191-246: The load_from_file method currently uses the deprecated
Pydantic v2 .dict() call when reconstructing the Config instance; update
load_from_file to call model_dump() instead (mirror the pattern used in
load_from_env), so replace the .dict() usage in load_from_file with model_dump()
when creating merged_config and then pass that to Config(...) or cls(...);
reference the load_from_file function and the merged_config variable to locate
the change.
In `@holmes/core/tracing.py`:
- Around line 343-359: OTELSpan.set_attributes currently turns None values into
empty strings which can mask missing attributes; update the method
(set_attributes) so that when iterating span_attributes it simply skips any key
whose value is None instead of calling self._span.set_attribute(key, ""); keep
the existing handling for str/int/float/bool and fallback to str(...) for other
types, and leave the name/type handling unchanged (i.e., still call
self._span.set_attribute("span.name", name) when name is provided).
- Around line 326-341: OTELSpan.log currently stringifies all metadata values;
change the metadata loop in the log method to preserve OTEL-native types by
passing ints, floats, and bools directly to self._span.set_attribute (and
strings after applying the existing _truncate). For any non-primitive types
(e.g., dicts, objects), fall back to str(value) before setting the attribute.
Keep using the same _span.set_attribute call and the existing _truncate behavior
only for string values so that numeric and boolean metadata remain typed for
better querying.
In `@holmes/utils/stream.py`:
- Around line 237-241: The hardcoded truncation value 8192 is duplicated and
should be replaced by a named constant; add a local constant (e.g.,
MAX_ATTR_SIZE or LOCAL_MAX_ATTRIBUTE_SIZE) near the top of
holmes/utils/stream.py and use it wherever tool_arguments
(data["tool_arguments"]) is truncated (and the similar truncation at lines
handling other attributes around 272-273) so the truncation uses that constant
instead of the magic number; update occurrences of the literal 8192 to reference
the new constant (keep the same numeric value).
- Line 325: The variable "data" in holmes/utils/stream.py is declared as data =
{} without a type annotation while similar builder variables use dict[str, Any];
update the declaration of data (the local variable in the builder function where
data is set) to include an explicit type hint (data: dict[str, Any] = {}) so it
matches the other annotated variables (see the other builder variables declared
as dict[str, Any]) and keeps typing consistent across the function(s) using
data.
In `@tests/test_bash_session_prefix_flow.py`:
- Around line 43-49: Remove the redundant reassignment of mock_response.usage to
a new MagicMock: since mock_response is already a MagicMock, its .usage child is
auto-created, so delete the line "mock_response.usage = MagicMock()" and leave
the subsequent assignments (mock_response.usage.prompt_tokens,
.completion_tokens, .total_tokens) as-is so the test still sets the token counts
while avoiding an unnecessary override of the existing mock object.
| ``` | ||
| ┌─────────────────────────────────────────────────────────────────┐ | ||
| │ Entry Points │ | ||
| ├─────────────────────────────────────────────────────────────────┤ | ||
| │ CLI (holmes ask) │ server.py │ AG-UI (server-agui.py) │ | ||
| └─────────┬──────────┴──────┬──────┴──────────┬────────────────────┘ | ||
| │ │ │ | ||
| ▼ ▼ ▼ | ||
| ┌─────────────────────────────────────────────────────────────────┐ | ||
| │ TracingFactory │ | ||
| │ ┌─────────────┐ ┌─────────────┐ ┌─────────────────────────┐ │ | ||
| │ │ Braintrust │ │ OTEL │ │ CompositeTracer │ │ | ||
| │ │ Tracer │ │ Tracer │ │ (dual tracing support) │ │ | ||
| │ └─────────────┘ └─────────────┘ └─────────────────────────┘ │ | ||
| └─────────────────────────────────────────────────────────────────┘ | ||
| │ │ | ||
| │ ▼ | ||
| │ ┌─────────────────────────────────────┐ | ||
| │ │ experimental/otel/ │ | ||
| │ │ ┌─────────┐ ┌─────────┐ ┌───────┐ │ | ||
| │ │ │tracing │ │metrics │ │logging│ │ | ||
| │ │ └─────────┘ └─────────┘ └───────┘ │ | ||
| │ └─────────────────────────────────────┘ | ||
| │ │ | ||
| ▼ ▼ | ||
| ┌─────────────────────────────────────────────────────────────────┐ | ||
| │ OTLP Exporter │ | ||
| │ (with optional AWS SigV4 for OSIS endpoints) │ | ||
| └─────────────────────────────────────────────────────────────────┘ | ||
| │ | ||
| ▼ | ||
| ┌─────────────────────────────────────────────────────────────────┐ | ||
| │ Observability Backend │ | ||
| │ (OpenSearch/OSIS, Jaeger, Zipkin, etc.) │ | ||
| └─────────────────────────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to the ASCII diagram and span hierarchy fences.
Markdownlint (MD040) flags these fenced blocks without a language. Use text/plaintext to keep rendering stable.
Also applies to: 85-93
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/otel-tracing.md` around lines 17 - 52, The fenced ASCII diagram blocks
(the triple-backtick blocks containing the "Entry Points / TracingFactory / OTLP
Exporter" diagram and the span hierarchy fences) are missing language
identifiers and trigger MD040; update each opening fence from ``` to ```text (or
```plaintext) so the diagram is treated as plain text; ensure you update the
main ASCII diagram block and the other affected fenced blocks (the second
occurrence referenced in the comment) so both render warnings are resolved.
| ## Troubleshooting | ||
|
|
||
| ### Common Issues | ||
|
|
||
| **"OTEL tracing requested but OTEL_SDK_DISABLED is not set to 'false'"** | ||
|
|
||
| Set `OTEL_SDK_DISABLED=false` in your environment. | ||
|
|
||
| **"Failed to create OSIS session"** | ||
|
|
||
| Check your AWS credentials and ensure `HOLMES_AWS_OSIS_PROFILE` points to a valid profile with OSIS permissions. | ||
|
|
||
| **"payload too large" errors** | ||
|
|
||
| Tool outputs are automatically truncated to 8KB. If you still see this error, check for large metadata values. | ||
|
|
||
| ### Debug Logging | ||
|
|
||
| Enable debug logging to see OTEL initialization details: | ||
|
|
||
| ```bash | ||
| LOG_LEVEL=DEBUG python server.py | ||
| ``` |
There was a problem hiding this comment.
Consolidate troubleshooting commands into a single annotated block.
The separate “Debug Logging” subheader is a small section. Consider replacing the subheader with bold text and keeping troubleshooting commands in one code block with comments.
As per coding guidelines: "docs/**/*.md: In MkDocs documentation, don't create a header for every small section. Use headers sparingly for major sections...andConsolidate troubleshooting commands in documentation into a single code block with comments instead of separate headers for each scenario`."
🧰 Tools
🪛 LanguageTool
[style] ~270-~270: Consider an alternative verb to strengthen your wording.
Context: ...atically truncated to 8KB. If you still see this error, check for large metadata va...
(IF_YOU_HAVE_THIS_PROBLEM)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/otel-tracing.md` around lines 256 - 278, Consolidate the small "Debug
Logging" subsection into the main "Troubleshooting" block: remove the separate
"Debug Logging" header and replace it with bold inline text (e.g., **Debug
logging**) and merge all troubleshooting commands into a single annotated code
block containing the environment and debug commands (e.g., setting
OTEL_SDK_DISABLED=false, checking HOLMES_AWS_OSIS_PROFILE, note about payload
truncation, and the LOG_LEVEL=DEBUG python server.py command) with short
comments for each line; edit the sections referencing the quoted issues ("OTEL
tracing requested but OTEL_SDK_DISABLED is not set to 'false'", "Failed to
create OSIS session", "payload too large") and the existing LOG_LEVEL example so
they appear as explanatory lines surrounding one unified fenced code block
instead of separate headers.
| def truncate(value: Optional[str], max_size: int = MAX_ATTRIBUTE_SIZE) -> str: | ||
| """Truncate a string value to prevent OTEL payload size errors. | ||
|
|
||
| Based on OTEL best practices for attribute size limits. | ||
| See: | ||
| - OTEL attribute limits: https://opentelemetry.io/docs/specs/otel/common/#attribute-limits | ||
|
|
||
| Args: | ||
| value: The string to truncate (can be None) | ||
| max_size: Maximum allowed size (default 8KB) | ||
|
|
||
| Returns: | ||
| Original string if within limits, otherwise truncated with marker. | ||
| Returns empty string if value is None. | ||
| """ | ||
| if value is None: | ||
| return "" | ||
| if len(value) <= max_size: | ||
| return value | ||
| # Account for marker length to stay within max_size | ||
| return value[: max_size - len(TRUNCATION_MARKER)] + TRUNCATION_MARKER |
There was a problem hiding this comment.
truncate produces output exceeding max_size when max_size < len(TRUNCATION_MARKER).
If a caller passes a small max_size (e.g., truncate("hello world", max_size=5)), the result would be "" + "...[TRUNCATED]" = "...[TRUNCATED]" (14 chars), exceeding the requested limit. While the default MAX_ATTRIBUTE_SIZE = 8192 makes this unlikely in practice, the function's contract says it respects max_size.
Proposed fix
def truncate(value: Optional[str], max_size: int = MAX_ATTRIBUTE_SIZE) -> str:
if value is None:
return ""
if len(value) <= max_size:
return value
- # Account for marker length to stay within max_size
- return value[: max_size - len(TRUNCATION_MARKER)] + TRUNCATION_MARKER
+ # Account for marker length to stay within max_size
+ if max_size <= len(TRUNCATION_MARKER):
+ return value[:max_size]
+ return value[: max_size - len(TRUNCATION_MARKER)] + TRUNCATION_MARKER🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@experimental/otel/attributes.py` around lines 158 - 178, The truncate
function can return a string longer than max_size when max_size is smaller than
len(TRUNCATION_MARKER); update truncate (referencing truncate,
TRUNCATION_MARKER, MAX_ATTRIBUTE_SIZE) to first handle non-positive max_size by
returning "" and then handle the small-max case by returning at most the first
max_size characters of TRUNCATION_MARKER when max_size <=
len(TRUNCATION_MARKER); otherwise keep the existing logic but ensure the sliced
prefix length is max_size - len(TRUNCATION_MARKER) so the final string never
exceeds max_size.
| def test_tracer_initialization(): | ||
| """Test that tracer initializes with OSIS endpoint.""" | ||
| print("\n" + "=" * 60) | ||
| print("Test: Tracer Initialization") | ||
| print("=" * 60) | ||
|
|
||
| # Reset module state for clean test | ||
| from experimental.otel import tracing | ||
|
|
||
| tracing._initialized = False | ||
| tracing._tracer_provider = None | ||
|
|
||
| from experimental.otel.tracing import init_otel_tracer | ||
|
|
||
| result = init_otel_tracer() |
There was a problem hiding this comment.
Move helper imports to module scope and add return type hints.
Several helpers import modules inside the function body and omit return annotations. Please hoist imports to the top and add -> bool/-> int (or -> None) as appropriate.
💡 Example adjustment
+from experimental.otel.tracing import init_otel_tracer
@@
-def test_tracer_initialization():
+def test_tracer_initialization() -> bool:
"""Test that tracer initializes with OSIS endpoint."""
@@
- from experimental.otel.tracing import init_otel_tracer
-
result = init_otel_tracer()As per coding guidelines: "**/*.py: Type hints required (mypy configuration in pyproject.toml)andALWAYS 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 `@experimental/otel/test_otel_integration.py` around lines 107 - 121, In
test_tracer_initialization and any helper functions in this test module that
currently import modules inside function bodies, move those imports to module
scope at the top of experimental/otel/test_otel_integration.py and remove inline
imports; also add explicit return type hints (e.g., -> bool, -> int or -> None)
to every helper function and test helper used in this file so signatures satisfy
the project's mypy rules; ensure the test still resets tracing._initialized and
tracing._tracer_provider and continues to call init_otel_tracer() unchanged.
| # Emit context check start event for OTEL tracing | ||
| yield build_stream_event_context_check(is_start=True) | ||
|
|
||
| limit_result = limit_input_context_window( | ||
| llm=self.llm, messages=messages, tools=tools | ||
| ) | ||
| yield from limit_result.events | ||
| messages = limit_result.messages | ||
| metadata = metadata | limit_result.metadata | ||
|
|
||
| # Emit context check end event for OTEL tracing | ||
| yield build_stream_event_context_check( | ||
| is_start=False, |
There was a problem hiding this comment.
Rephrase OTEL event comments to capture the rationale.
Comments like “Emit context check start event for OTEL tracing” restate the code. Consider explaining the intent (e.g., correlating context trimming with trace spans) or removing them.
As per coding guidelines: "**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Write clear, concise comments that explain 'why' rather than 'what'`."
🤖 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 953 - 965, The inline comments
around the context-check events are restating the code; change them to explain
the rationale: clarify that the calls to build_stream_event_context_check before
and after limit_input_context_window are used to create OTEL trace spans/events
that correlate context-trimming operations (limit_input_context_window) with the
surrounding stream processing for observability and debugging; update the two
comments adjacent to build_stream_event_context_check and the
limit_input_context_window call to mention intent (e.g., "start/end OTEL event
to correlate context trimming with trace spans for visibility into message
windowing performed by limit_input_context_window") so future readers understand
why the events are emitted.
| class CompositeSpan: | ||
| """Span that delegates to multiple underlying spans.""" | ||
|
|
||
| def __init__(self, spans: List[Union[OTELSpan, DummySpan, Any]]): | ||
| self._spans = spans | ||
|
|
||
| def start_span( | ||
| self, name: Optional[str] = None, span_type: Optional[SpanType] = None, **kwargs | ||
| ) -> "CompositeSpan": | ||
| """Create child spans on all underlying spans.""" | ||
| child_spans = [s.start_span(name, span_type, **kwargs) for s in self._spans] | ||
| return CompositeSpan(child_spans) | ||
|
|
||
| def log(self, *args, **kwargs) -> None: | ||
| """Log to all underlying spans.""" | ||
| for span in self._spans: | ||
| span.log(*args, **kwargs) | ||
|
|
||
| def set_attributes(self, **kwargs) -> None: | ||
| """Set attributes on all underlying spans.""" | ||
| for span in self._spans: | ||
| span.set_attributes(**kwargs) | ||
|
|
||
| def end(self) -> None: | ||
| """End all underlying spans.""" | ||
| for span in self._spans: | ||
| span.end() | ||
|
|
||
| def __enter__(self) -> "CompositeSpan": | ||
| return self | ||
|
|
||
| def __exit__(self, exc_type, exc_val, exc_tb) -> None: | ||
| for span in self._spans: | ||
| span.__exit__(exc_type, exc_val, exc_tb) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all start_span implementations in the file and related files
rg -n 'def start_span' --type=py -B 2 -A 8Repository: HolmesGPT/holmesgpt
Length of output: 4193
🏁 Script executed:
# Check the SpanType enum definition
rg -n 'class SpanType\|SpanType\s*=' --type=py -B 2 -A 5Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Look for CompositeSpan instantiation and usage
rg -n 'CompositeSpan\(' --type=py -B 2 -A 2Repository: HolmesGPT/holmesgpt
Length of output: 2929
🏁 Script executed:
# Check if Braintrust is integrated and how
rg -n 'braintrust\|BraintrustSpan' --type=py -B 2 -A 5Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Search for any braintrust references
fd . -type f -name "*.py" -o -name "*.toml" -o -name "*.txt" | xargs rg -i "braintrust" 2>/dev/null || echo "No braintrust found"Repository: HolmesGPT/holmesgpt
Length of output: 50565
🏁 Script executed:
# Check if there are other span implementations besides OTELSpan and DummySpan
rg -n 'class.*Span.*:' --type=py | grep -E 'Span|Tracer'Repository: HolmesGPT/holmesgpt
Length of output: 1170
🏁 Script executed:
# Check dependencies in pyproject.toml or requirements files
find . -type f \( -name "pyproject.toml" -o -name "requirements*.txt" -o -name "setup.py" \) -exec grep -l "braintrust\|Braintrust" {} \;Repository: HolmesGPT/holmesgpt
Length of output: 79
🏁 Script executed:
# Look at the type hint context more carefully - what is imported and used
head -50 holmes/core/tracing.py | grep -E 'import|from|Union|List|Any'Repository: HolmesGPT/holmesgpt
Length of output: 729
🏁 Script executed:
# Check TracingFactory and how spans are composed
rg -n "TracingFactory\|create_tracer\|CompositeTracer" --type py -B 2 -A 5 holmes/Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Look for any code that constructs CompositeSpan with Braintrust spans
rg -n "CompositeSpan.*braintrust\|braintrust.*CompositeSpan" --type py -B 3 -A 3Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Check what tracers can create Braintrust spans
rg -n "class.*Tracer.*:" --type py | head -20Repository: HolmesGPT/holmesgpt
Length of output: 420
🏁 Script executed:
# Check how tracers are created and composed
head -100 holmes/core/tracing.py | tail -50Repository: HolmesGPT/holmesgpt
Length of output: 1602
🏁 Script executed:
# Look at BraintrustTracer.start_trace and what it returns
sed -n '175,250p' holmes/core/tracing.pyRepository: HolmesGPT/holmesgpt
Length of output: 2506
🏁 Script executed:
# Look at CompositeTracer.start_trace to see how it composes spans
sed -n '502,540p' holmes/core/tracing.pyRepository: HolmesGPT/holmesgpt
Length of output: 1377
🏁 Script executed:
# Check what BraintrustTracer returns from start_trace/start_span
rg -n "class BraintrustTracer" -A 60 holmes/core/tracing.pyRepository: HolmesGPT/holmesgpt
Length of output: 2117
🏁 Script executed:
# Check if CompositeTracer is ever used with both OTELTracer and BraintrustTracer
rg -n "CompositeTracer\|both.*tracer\|dual.*trac" --type py -B 3 -A 3 holmes/ tests/Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Look at test_otel_tracing to see if composite spans with mixed types are tested
sed -n '114,150p' tests/test_otel_tracing.pyRepository: HolmesGPT/holmesgpt
Length of output: 1467
🏁 Script executed:
# Check what the Braintrust Span object signature is in the type hints
rg -n "Span.*start_span\|start_span.*Span" --type py -B 2 -A 5 holmes/Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Look for type hints or imports of braintrust.Span
rg -n "from braintrust import\|braintrust.Span" --type py -B 2 -A 5Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Check if there's any documentation or comments about Braintrust Span.start_span signature
rg -n "type.*string\|span_type.*enum\|Braintrust.*start_span" --type py -B 2 -A 2Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Look at the actual usage pattern - does BraintrustTracer ever create nested spans?
sed -n '215,280p' holmes/core/tracing.pyRepository: HolmesGPT/holmesgpt
Length of output: 2573
🌐 Web query:
Braintrust SDK Span.start_span signature parameter name type string
💡 Result:
In the Braintrust Python SDK, Span.start_span(...) takes:
name: str | Nonetype: SpanTypeAttribute | None(i.e., a restricted string type, used for UI display/icons—not an arbitrary string) [1]
type is typically one of: "llm", "score", "function", "eval", "task", or "tool" (these are the recognized span type values in span_attributes). [2]
Sources: [1] (braintrust.dev) [2] (braintrust.dev)
Citations:
- 1: https://www.braintrust.dev/docs/reference/sdks/python?utm_source=openai
- 2: https://www.braintrust.dev/docs/core/logs/advanced?utm_source=openai
CompositeSpan.start_span passes span_type positionally but Braintrust Span expects type as parameter name.
Line 476 passes span_type (a SpanType enum) as the second positional argument to all underlying spans. For DummySpan and OTELSpan, the second parameter is also named span_type, which matches. However, when CompositeSpan contains a Braintrust Span object (returned by BraintrustTracer.start_trace), the positional argument mismatches the expected parameter name: Braintrust's Span.start_span signature expects type as the parameter name, not span_type. Additionally, Braintrust expects type to be a string value from a restricted set ("llm", "score", "function", "eval", "task", "tool"), while passing a SpanType enum positionally would cause unexpected behavior.
Consider refactoring to use keyword-based delegation: s.start_span(name=name, span_type=span_type, **kwargs) or normalize the argument for each span type in a type-aware manner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/core/tracing.py` around lines 466 - 499, CompositeSpan.start_span
currently passes span_type positionally which breaks Braintrust's
Span.start_span expecting a named parameter type (string); change the delegation
to call start_span with explicit keywords (e.g. s.start_span(name=name,
span_type=span_type, **kwargs)) and handle Braintrust spans by detecting that
span object (or by duck-typing on the start_span signature) and passing
type=<mapped_string> where <mapped_string> converts your SpanType enum to one of
Braintrust's allowed strings ("llm","score","function","eval","task","tool");
update CompositeSpan.start_span to branch per span kind (OTELSpan/DummySpan use
span_type=..., Braintrust Span use type=<mapped_string>) so calls match each
implementation's parameter names and types.
| def init_otel(): | ||
| """Initialize OTEL tracing and metrics for production observability if enabled. | ||
|
|
||
| Metrics are initialized here alongside tracing. The HTTP middleware (below) | ||
| handles per-request span creation; metrics recording happens in the AG-UI | ||
| endpoint and tool execution paths. | ||
| """ | ||
| if os.environ.get("OTEL_SDK_DISABLED", "true").lower() != "true": | ||
| # Initialize tracing | ||
| tracing_ok = TracingFactory.init_otel() | ||
| if tracing_ok: | ||
| logging.info("OTEL tracing enabled for Holmes server") | ||
| else: | ||
| logging.warning("OTEL tracing initialization failed") | ||
|
|
||
| # Initialize metrics | ||
| try: | ||
| from experimental.otel.metrics import init_otel_metrics | ||
|
|
||
| metrics_ok = init_otel_metrics() | ||
| if metrics_ok: | ||
| logging.info("OTEL metrics enabled for Holmes server") | ||
| else: | ||
| logging.warning("OTEL metrics initialization failed") | ||
| except ImportError: | ||
| logging.warning("OTEL metrics module not available") | ||
| metrics_ok = False | ||
|
|
||
| return tracing_ok or metrics_ok |
There was a problem hiding this comment.
Add a return type and hoist the optional metrics import to module scope.
init_otel has no return annotation, and importing init_otel_metrics inside the function breaks the “imports at top” rule. A top-level try/except ImportError keeps the dependency optional while satisfying the guideline.
💡 Suggested update
+try:
+ from experimental.otel.metrics import init_otel_metrics
+except ImportError:
+ init_otel_metrics = None
+
-def init_otel():
+def init_otel() -> bool:
@@
- # Initialize metrics
- try:
- from experimental.otel.metrics import init_otel_metrics
-
- metrics_ok = init_otel_metrics()
+ # Initialize metrics
+ metrics_ok = False
+ if init_otel_metrics:
+ metrics_ok = init_otel_metrics()
if metrics_ok:
logging.info("OTEL metrics enabled for Holmes server")
else:
logging.warning("OTEL metrics initialization failed")
- except ImportError:
- logging.warning("OTEL metrics module not available")
- metrics_ok = False
+ else:
+ logging.warning("OTEL metrics module not available")As per coding guidelines: "**/*.py: Type hints required (mypy configuration in pyproject.toml)andALWAYS 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 `@server.py` around lines 63 - 91, The function init_otel lacks a return type
annotation and performs a local import; hoist the optional import by adding a
module-level try/except ImportError that sets a module symbol (e.g.,
init_otel_metrics = None) so the dependency remains optional, then update
init_otel to declare a return type (-> bool) and use the hoisted
init_otel_metrics symbol (check for None before calling) to determine
metrics_ok; ensure the function always returns a boolean (True if tracing_ok or
metrics_ok, False otherwise) and preserves the existing logging behavior.
| class TestOTELAttributes: | ||
| def test_truncate_none_returns_empty(self): | ||
| from experimental.otel.attributes import truncate | ||
|
|
||
| assert truncate(None) == "" | ||
|
|
||
| def test_truncate_short_string_unchanged(self): | ||
| from experimental.otel.attributes import truncate | ||
|
|
||
| short = "hello world" | ||
| assert truncate(short) == short | ||
|
|
||
| def test_truncate_exact_limit_unchanged(self): | ||
| from experimental.otel.attributes import MAX_ATTRIBUTE_SIZE, truncate |
There was a problem hiding this comment.
Hoist per-test imports and add -> None return annotations.
Several tests import modules inside the function body and omit return annotations. Please move these imports to module scope and add -> None to test methods for mypy compliance.
💡 Example adjustment
+from experimental.otel.attributes import truncate
@@
- def test_truncate_none_returns_empty(self):
- from experimental.otel.attributes import truncate
+ def test_truncate_none_returns_empty(self) -> None:
assert truncate(None) == ""As per coding guidelines: "**/*.py: Type hints required (mypy configuration in pyproject.toml)andALWAYS 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 `@tests/test_otel_tracing.py` around lines 193 - 206, In TestOTELAttributes,
hoist per-test imports (e.g., from experimental.otel.attributes import truncate,
MAX_ATTRIBUTE_SIZE) out of each test function to the module top and add explicit
return type annotations to each test method (e.g., def
test_truncate_none_returns_empty(self) -> None, def
test_truncate_short_string_unchanged(self) -> None, def
test_truncate_exact_limit_unchanged(self) -> None) so mypy sees top-level
imports and typed test methods; update all test methods in this class that
import truncate or MAX_ATTRIBUTE_SIZE inside the body accordingly.
Add OTEL instrumentation to the experimental AG-UI chat endpoint for observability and distributed tracing. Traces are exported to AWS OSIS (OpenSearch Ingestion Service) using SigV4 authentication. Signed-off-by: Megha Goyal <goyamegh@amazon.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com>
…rics - Added LoggingSpanProcessor and LoggingSpanExporter to log span lifecycle events and export results for debugging. - Implemented metrics helpers that are no-op when OTEL is disabled. - Introduced structured logging helpers for LLM calls and tool executions. - Created new stream events for LLM iterations to support OTEL tracing. - Updated TracingFactory to support dual tracing with Braintrust and OpenTelemetry. - Enhanced server.py to initialize OTEL tracing and added middleware for HTTP request spans. - Refactored configuration loading to merge environment variables with config file settings. Signed-off-by: Megha Goyal <goyamegh@amazon.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com>
… handling Signed-off-by: Megha Goyal <goyamegh@amazon.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com>
…lemetry spans Signed-off-by: Megha Goyal <goyamegh@amazon.com>
…s for improved tracing Signed-off-by: Megha Goyal <goyamegh@amazon.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com>
Replace inline import in _get_version() with importlib.metadata.version() to avoid circular import while keeping imports at top-level. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com>
…ments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com>
- Rename OTEL_ENABLED -> OTEL_SDK_DISABLED (per OTEL spec, inverted logic) - Rename OTEL_METRICS_ENABLED -> OTEL_METRICS_EXPORTER=none (per OTEL spec) - Rename OTEL_AWS_PROFILE -> HOLMES_AWS_OSIS_PROFILE (remove OTEL_ prefix) - Rename OTEL_AWS_REGION -> HOLMES_AWS_OSIS_REGION (remove OTEL_ prefix) - Update docs, tests, and all references Addresses review comments from svrnm (OpenTelemetry maintainer): - OTEL_SDK_DISABLED is the official spec variable for disabling the SDK - AWS-specific variables must not use the OTEL_ namespace prefix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com>
- Rename OTEL_AWS_SERVICE → HOLMES_AWS_OSIS_SERVICE with backwards-compat fallback (Comment HolmesGPT#10, svrnm) - Align OTEL_DEBUG with OTEL spec OTEL_LOG_LEVEL=debug with backwards-compat fallback (Comment HolmesGPT#9, svrnm) - Add ml-commons AgentTracer.java GitHub permalink (Comment HolmesGPT#3, kylehounslow) - Document needs_aws_auth() as single source of truth with consumer list (Comment HolmesGPT#4, kylehounslow) - Clarify otel_logging.py: logs NOT exported via OTLP, naming avoids shadowing builtin logging (Comment HolmesGPT#5, kylehounslow) - Explain experimental/ placement: API evolving, removable via try/except no-op fallbacks (Comment HolmesGPT#6, kylehounslow) - Clarify server.py middleware: tracing/metrics init in init_otel() above, middleware only handles per-request spans (Comment HolmesGPT#7, kylehounslow) - Split env var docs into Standard OTEL / Holmes-Specific tables, add missing vars, add OSIS hyperlink (Comments HolmesGPT#1, HolmesGPT#9, HolmesGPT#10) - Fix no-op tracer fallback in server-agui.py for when OTEL is unavailable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Megha Goyal <goyamegh@amazon.com>
24326fc to
b65c52a
Compare
|
Hey, Thanks for the PR |
6 participants
Summary
Add OpenTelemetry (OTEL) instrumentation to the experimental AG-UI chat endpoint for observability and distributed tracing. Traces are exported to AWS OSIS (OpenSearch Ingestion Service) using SigV4 authentication.
Motivation
Key changes:
Environment variables:
Changes
New Module:
experimental/otel/tracing.py: Tracer initialization with AWS SigV4 auth for OSIS endpointsattributes.py: Gen AI semantic convention attribute constants (REQUEST_ID, TOOL_NAME, etc.)__init__.py: Package exportsAG-UI Integration (
server-agui.py)agent.run) created for each chat request with correlation attributestool.execute) for each tool call with duration trackingTests
test_otel.py: Unit tests (no external dependencies required)test_otel_integration.py: Integration tests with real OSIS endpoint + OpenSearch verificationDependencies
opentelemetry-api,opentelemetry-sdk,opentelemetry-exporter-otlp-proto-httpConfiguration
Environment variables (all optional - tracing disabled by default):
OTEL_ENABLEDtrueto enable tracingOTEL_EXPORTER_OTLP_ENDPOINThttps://xxx.us-east-1.osis.amazonaws.com/path/v1/traces)OTEL_AWS_PROFILEOTEL_AWS_REGIONOTEL_SERVICE_NAMEholmesgpt)Testing
Unit Tests
Integration Tests (with real OSIS endpoint)
Notes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores