Repository navigation
fix(otel): reject competing telemetry views - #753
zhongkechen wants to merge 15 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| f"Durable instrumentation plugin {_qualified_class_name(plugin_type)} " | ||
| "must declare exclusive_group as None or a non-empty string." | ||
| ) | ||
| if (previous := groups.get(group)) is not None: |
There was a problem hiding this comment.
Repeated registrations of one concrete class in an exclusive group should be rejected, and this already does it, since _validate_exclusive_groups keys on the group without comparing classes. There is no test covering it, so nothing pins the behaviour.
The two paths differ. A duplicate inside plugins=[...] is rejected here. A duplicate across paths is not, because registered_types is pre-seeded from the explicit list, so an environment-selected provider of an already-registered type is skipped. Both deserve a test.
Minor. When the repeated class is the same, the diagnostic reads "plugins X and X are mutually exclusive". A separate branch could say to register it once.
There was a problem hiding this comment.
Fixed in 3f0fc46. Added four regression cases: repeated explicit instances of the same exclusive class are rejected (both the same object and distinct objects); an environment provider of an explicitly registered type is skipped without calling its factory; and two provider names for the same exclusive type keep only the first registration. The explicit/environment precedence remains the existing documented behavior. The same-class diagnostic now says to register the plugin only once instead of saying that X and X are mutually exclusive.
Validation: all 1,744 core tests (plus 5 subtests), all 67 plugin-discovery cases, and 35 OTel registration/lifecycle tests pass. Core type checking, package Hatch lint/format, commit lint, and diff checks pass. Current-head CI is starting.
| A marker on a base class intentionally does not opt in its subclasses. | ||
| Legacy attributes named exclusive_group/on_registration_result stay inert. | ||
| """ | ||
| namespace = type.__dict__["__dict__"].__get__(plugin_type, type(plugin_type)) |
There was a problem hiding this comment.
This reads the marker from the class's own __dict__, so it is not inherited. A subclass of a bundled view returns False here, _validate_exclusive_groups hits continue at line 154, and its inherited exclusive_group is never read. So plugins=[MyExecutionSubclass(), InvocationOtelPlugin()] loads both views, which is the case #652 reports.
Subclasses should be checked. Walking the MRO and consulting each class's own __dict__ keeps the legacy protection, since the first class declaring the marker supplies the group and a plugin with a coincidental exclusive_group and no marker anywhere stays inert.
Line 209 gates on_registration_result the same way, so such a subclass would also start receiving the callback, which it needs for log-filter cleanup. The README paragraph stating the marker is never inherited needs updating.
Three tests assert the old behaviour and would flip. test_registration_opt_in_is_not_inherited_by_existing_subclasses (721) and test_legacy_otel_subclass_is_not_implicitly_opted_in (tests/e2e/test_view_registration.py:363) both expect a subclass to be inert. test_only_literal_registration_api_version_opts_in (738) sets an invalid marker on a subclass whose parent declares a valid one, so the walk needs explicit semantics on whether an invalid marker shadows the parent.
New coverage needed is a subclass of one view rejected against the opposite view, and a subclass inheriting the marker receiving on_registration_result.
There was a problem hiding this comment.
Fixed in 997a44a. Registration now walks the MRO and selects the closest class declaring __durable_registration_api__. Subclasses of both bundled views inherit their exclusive group and cleanup callback, so the subclass/opposite-view combination is rejected before discovered factories run.
The declaring class supplies the inherited metadata and callback, bound to the subclass instance. This also preserves older subclasses with coincidental same-named fields/helpers: those are not inspected or called unless the subclass explicitly repeats the marker to customize the contract. An explicit value other than integer 1 shadows the inherited marker; it does not fall through to the parent. Both READMEs now describe these rules.
Validation: 3 core regressions and 12 real OTel subclass cases fail on the preceding implementation and pass with the fix. The full core suite passes 1,747 tests plus 5 subtests at 98.28% coverage; all 344 OTel tests and 14 compatibility cases against installed core 2.0.1 pass. Repository types, both packages' lint/format, commit lint and diff checks pass. The dependency floor and generic plugin base remain unchanged.
| for value in dependencies | ||
| if Requirement(value).name == "aws-durable-execution-sdk-python" | ||
| ) | ||
| assert requirement.specifier.contains("2.0.0") |
There was a problem hiding this comment.
These assertions hold the floor at core 2.0.0, so pip install aws-durable-execution-sdk-python-otel==1.1.0 against an installed core 2.0.x resolves cleanly and silently provides no exclusivity validation. A customer who upgrades the OTel package to pick up this fix, and nothing else, still gets both views loading.
So OTel 1.1.0 does not enforce exclusivity on every core version it declares support for. Raising the floor to >=2.1.0 would guarantee the behaviour at the cost of dropping 2.0.x support. If keeping 2.0.x is intentional, worth saying so in the PR description, since a reader will reasonably assume installing the fixed package applies the fix.
There was a problem hiding this comment.
Keeping core 2.0.x support is intentional. Upgrading only OTel on a 2.0.x core does not activate exclusivity; customers need the coordinated core 2.1+/OTel 1.1+ pair for that behavior. The older combination continues to support valid tracing configurations. I made the OTel-only upgrade limitation explicit in the PR description, alongside the unchanged dependency floor. The published-core lane remains enforced in CI, and I just reran its 14 registration/wait-resume cases against installed core 2.0.1 successfully.
|
#652 raised fail-fast versus error-and-drop as a decision to settle in the issue, and named the cost of fail-fast directly, that a customer already running both views starts failing on an SDK upgrade. It also suggested a middle option, fail fast on the explicit This implements fail-fast on both paths with no stated reasoning, so that trade-off is closed implicitly. Worth a line in the PR description on why fail-fast won for the environment path, and whether anyone running both views today was considered. |
|
@nvasiu Added the failure-mode rationale and upgrade impact to the PR description. The environment path deliberately fails fast for the same reason as explicit registration: silently dropping one of two configured views would choose the application's telemetry view and parent/correlation behavior on its behalf. The validation instead reports both conflicting plugins before discovered factories run, so the configuration can be corrected explicitly. The compatibility cost is real: a deployment enabling both views will fail handler initialization when upgraded to the coordinated newer core/OTel pair. It needs to select one view before upgrading. The PR now says this directly and does not assume that no existing deployments use both. Upgrading OTel alone while keeping core 2.0.x retains the old behavior; that limitation is also explicit. |
| concurrency: | ||
| group: otel-conformance-tests | ||
| cancel-in-progress: false | ||
| queue: max |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_bsc5lxoxpqaaru6j2vtvgojbll
[P1] Remove the unsupported queue key. GitHub Actions concurrency accepts group and cancel-in-progress; queue: max makes this workflow fail validation before any job runs. The same key was added to cloud-tests.yml and conformance-tests.yml, disabling those workflows too. Remove all three keys, and use an external locking/dispatch mechanism if every pending run must be retained.
There was a problem hiding this comment.
Checked against the current official Actions workflow schema: concurrency-mapping includes queue, with allowed values single and max. Both workflow-level and job-level concurrency use that mapping.
Runtime validation agrees: Cloud tests and Conformance Tests succeeded on the reviewed commit 68166eb; all six OTel test jobs also succeeded with these same concurrency declarations before the subsequent JS harness-only commit. These workflows are therefore accepted and executing. Keeping queue: max with cancel-in-progress: false preserves pending runs without canceling the active tests.
Codex AI reviewOne blocking CI configuration issue was found; no confirmed SDK runtime regressions otherwise. Reviewed commit |
Fixes #652 for the coordinated core 2.1+/OTel 1.1+ capability pair.
The two bundled OTel views, including custom subclasses, are rejected together at handler initialization before discovered factories run. Repeating the same exclusive class in the explicit list also raises a clear “register once” diagnostic. Environment-selected providers retain the existing first-registration precedence, so an explicitly registered concrete type is not constructed again.
The environment path deliberately uses the same fail-fast rule as explicit registration: dropping one configured view would silently choose the application's telemetry view and parent/correlation behavior. The diagnostic names the conflicting plugins before discovered factories run. A deployment already enabling both views will fail initialization after upgrading to the coordinated pair; select one view in configuration before upgrading. The PR makes no assumption that such deployments do not exist.
Registration is an opt-in contract declared with
__durable_registration_api__ = 1. The closest declaring class in the MRO supplies its group and cleanup callback. Subclasses inherit that contract while coincidental legacy fields/helpers with the same names remain inert; repeating the marker intentionally customizes it. An explicit marker other than integer1shadows inherited opt-in. Class namespace/MRO detection bypasses metaclass descriptors, and the generic plugin base gains no attributes or hooks.Rejected instances release their own constructor-installed logging filter; previously accepted resources are preserved. Reaccepting the same rejected OTel instance immediately restores enrichment when enabled.
Compatibility: OTel continues to support core
>=2.0.0, provider API version remains 1, and existing lifecycle ordering and checkpoint/replay formats are unchanged. Released core 2.0.x retains its existing tracing behavior; upgrading only OTel on core 2.0.x does not enable exclusivity. Install the coordinated newer core/OTel pair to get this validation. No plugin-factory migration is included.Validation: