feat: exercise real telemetry and Rich integrations - #37
Conversation
| TRACER_PROVIDER.add_span_processor(SimpleSpanProcessor(SPAN_EXPORTER)) | ||
| else: | ||
| SPAN_EXPORTER = None | ||
| TRACER_PROVIDER = None |
There was a problem hiding this comment.
Correctness: unguarded OTel SDK import/setup breaks base_cli's "broken plugin never changes completion" contract
This module-level block (imports of opentelemetry.sdk.*, plus InMemorySpanExporter()/TracerProvider()/add_span_processor() construction) runs at import time with no try/except, entirely outside of base_cli's start_telemetry/finish_telemetry, which are the only places the framework wraps OTel calls in except BaseException. The library's own docstring (base_cli/integrations.py) states: "every integration boundary is best-effort so a missing or broken plugin cannot change command completion" — and it even has a test named test_missing_or_broken_telemetry_never_changes_completion proving this is load-bearing.
pyproject.toml pins opentelemetry-api and opentelemetry-sdk independently (both >=1.24,<2), so a resolver could select mismatched minor versions in some environment. If that (or any other partial/broken opentelemetry-sdk install where find_spec succeeds but the internal wiring is inconsistent) happens, this import/construction raises uncaught and crashes the whole CLI invocation before status() even runs — instead of gracefully falling back to telemetry=unavailable. That directly contradicts the guarantee this demo package exists to showcase.
Suggest wrapping this setup in try/except and falling back to SDK_AVAILABLE = False on any failure, consistent with the rest of the optional-integration pattern in this file.
| click.echo(f"recorded_spans={len(spans)}") | ||
| for span in spans: | ||
| click.echo(f"span={span.name} status={span.status.status_code.name}") | ||
| return exit_code |
There was a problem hiding this comment.
Correctness: unguarded post-run flush/read can crash the CLI after the command already succeeded
TRACER_PROVIDER.force_flush() and SPAN_EXPORTER.get_finished_spans() here have no try/except, unlike every other OTel touchpoint base_cli owns (start_telemetry/finish_telemetry/_safe_span_call all wrap calls in except BaseException, per the framework's "broken plugin can't change command completion" contract). If either call raises for any reason in a real/degraded OTel install, the exception propagates out of main() after base_cli.run_app(command) has already returned a successful exit_code — turning a successful command into an unhandled traceback and non-zero process exit. Consider wrapping this block the same way base_cli wraps its own span calls.
| TELEMETRY_AVAILABLE = importlib.util.find_spec("opentelemetry") is not None | ||
| API_AVAILABLE = importlib.util.find_spec("opentelemetry") is not None | ||
| SDK_AVAILABLE = API_AVAILABLE and importlib.util.find_spec("opentelemetry.sdk") is not None | ||
| TELEMETRY_AVAILABLE = SDK_AVAILABLE |
There was a problem hiding this comment.
Simplification: TELEMETRY_AVAILABLE is a pure alias of SDK_AVAILABLE
TELEMETRY_AVAILABLE = SDK_AVAILABLE never diverges from SDK_AVAILABLE anywhere in this file (only used once, at line 35). Likewise API_AVAILABLE is only read once, to compute SDK_AVAILABLE. Three overlapping booleans for what's really two states (API present / SDK present) is harder to follow than it needs to be — consider dropping TELEMETRY_AVAILABLE and using SDK_AVAILABLE directly at the one call site.
1 participant
Fixes #25