aws / aws/aws-durable-execution-sdk-python
[otel] Allow only one OTel plugin at a time: document and validate
- Lenguaje dominante
- Python
- Estrellas
- 53
- Forks
- 25
- Merge medio
- 1 d 17 h
- PR fusionados (30 d)
- 37
Descripción
## Summary
`InvocationOtelPlugin` and `ExecutionOtelPlugin` are two views of the same telemetry and are not meant to run together, but nothing stops a customer from enabling both. Both providers ship as entry points, so `DURABLE_EXECUTION_PLUGINS=otel-invocation,otel-execution` loads both, and `durable_execution(plugins=[InvocationOtelPlugin(), ExecutionOtelPlugin()])` registers both. We have decided this is unsupported. It needs to be stated in the docs and enforced by the SDK instead of silently producing broken telemetry.
## Why this is not supported
- **Duplicated spans and export volume.** Each plugin creates its own Workflow and Invocation spans for the same execution, so every operation is exported twice under two different parents.
- **Conflicting OTel context scopes.** Both plugins attach the OTel context on the same thread in `on_user_function_start`. During user code the last writer wins, so each plugin's log correlation can resolve the other plugin's span.
- **Non-LIFO unwinding leaves an ended span current.** `PluginExecutor.execute_plugins` dispatches both start and end hooks in registration order, so the scopes are not unwound in reverse. Plugin A attaches token A, plugin B attaches token B; A detaches first, restoring the pre-invocation context, then B's detach restores ctxA — plugin A's just-ended span. Both plugins' registries are empty and every attach was paired with a detach, yet the thread is left with an ended span current. `ContextVar.reset` permits out-of-order resets without error, so nothing warns. Found by the automated review on #650 (P2); see https://github.com/aws/aws-durable-execution-sdk-python/pull/650.
## Acceptance criteria
### 1. Documentation
- `packages/aws-durable-execution-sdk-python-otel/README.md` states explicitly that exactly one OTel plugin may be active, with a short rationale and guidance on choosing between them. Today the wording is "select either OTel plugin by entry-point name" (lines 28-39), which reads as a preference rather than a constraint.
- The plugin section of `packages/aws-durable-execution-sdk-python/README.md` states the same constraint for both the `DURABLE_EXECUTION_PLUGINS` path and the `plugins=[...]` argument.
- Whatever error message the validation emits names both offending plugins and tells the reader to keep one, so the docs and the runtime message agree.
- The Developer Guide (https://docs.aws.amazon.com/durable-execution/) is maintained outside this repo; a doc ticket there may be needed for the observability/logging page.
### 2. Validation
- Enabling both OTel plugins is rejected at cold start, on both registration paths:
- the environment path in `plugin_discovery.load_configured_plugins` (`DURABLE_EXECUTION_PLUGINS=otel-invocation,otel-execution`)
- the explicit path (`durable_execution(plugins=[...])`), including the mixed case where one comes from the environment and the other from the decorator
- The existing duplicate guard is not sufficient: `load_configured_plugins` only skips a provider whose **concrete type** is already registered (`plugin_discovery.py:195-207`), and these are two different types, so both load today.
- The check produces a single clear diagnostic. `PluginLoadError` (`exceptions.py:65`) is the existing failure type for the discovery path.
### Implementation options to settle in this issue
**Where the mutual exclusion is expressed.** The core SDK should not hardcode OTel class names. Two candidates:
- Add an optional exclusivity/family field to `DurableInstrumentationPluginProvider` (a frozen dataclass, so an optional field is backwards compatible) and have discovery reject two providers in the same family. This does not cover the explicit `plugins=[...]` path, which has no provider.
- Declare the family on the plugin class (for example a class-level attribute read by `PluginExecutor` or by `durable_execution`), which covers both paths uniformly.
**Failure mode.** Fail fast versus warn-and-drop is a real trade-off:
- Fail fast at cold start is unambiguous and catches the misconfiguration immediately, but a customer already running both would start failing on an SDK upgrade.
- Log an error and keep only the first registration keeps the function running with correct (single-view) telemetry, but a misconfiguration can go unnoticed.
A middle option is fail fast for the explicit `plugins=[...]` path, which a developer sees in tests, and error-and-drop for the environment path, which is a deployment-time setting.
### 3. Tests
- Unit tests for `plugin_discovery`: both entry-point names configured, one from each path, and the still-supported single-plugin cases.
- A test asserting the diagnostic names both plugins.
- If the family/exclusivity metadata approach is taken, a test that an unrelated third-party plugin alongside one OTel plugin is still allowed.
## References
- #643 — context attach/detach balance, where the ordering interaction surfaced
- #650 — PR for #643; the review comment describing the non-LIFO detach across two plugins
- `packages/aws-durable-execution-sdk-python/src/aws_durable_execution_sdk_python/plugin_discovery.py`
- `packages/aws-durable-execution-sdk-python/src/aws_durable_execution_sdk_python/plugin.py` (`PluginExecutor.execute_plugins`)
Guía de contribución
Línea de trabajo
Start with plugin_discovery.py, plugin.py, and the PluginLoadError definition, then inspect the two README plugin sections and existing plugin discovery tests. Trace both environment and explicit registration paths, resolve the exclusivity and failure-mode choices, and verify that single-plugin cases, mixed registration, diagnostics naming both plugins, and unrelated plugins are covered.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- python
- Área
- backend-api-design, observability-sre
- Tipo de issue
- Nueva funcionalidad
- Dificultad
- 5/5
- Tiempo estimado
- Más de una semana
- Estado de actividad
- Activo
- Claridad
- Bastante claro
- Aptitud para principiantes
- 42/100