Repository navigation
fix(otel): reject competing telemetry views - #753
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 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
This comment has been minimized.
This comment has been minimized.
| if owner is None: | ||
| continue | ||
| try: | ||
| group = getattr(owner, "exclusive_group", None) |
There was a problem hiding this comment.
owner is the single closest class declaring the marker, so only that class's exclusive_group is read. A subclass that repeats the marker and declares its own group therefore loses the group it inherited.
PlainSubclass owner=ExecutionOtelPlugin group=aws-durable-execution-otel-view
SubclassWithOwnGroup owner=SubclassWithOwnGroup group=my-own-group
So plugins=[SubclassWithOwnGroup(), InvocationOtelPlugin()] records two different groups and loads both views, which is the configuration #652 reports. A subclass of a bundled view should stay in that view's group whatever else it declares.
Collecting every exclusive_group declared in the MRO, and checking the plugin against each, closes that. Note this is a separate decision from marker shadowing, which should stay as it is: an invalid marker on a subclass makes the class inert, while a valid marker plus a new group should add a group rather than replace one.
No test covers a subclass declaring its own group. test_subclass_inherits_exclusive_group_before_factory_runs covers a subclass that declares nothing. Worth a case asserting the re-declaring subclass is rejected against both the inherited group and its own.
There was a problem hiding this comment.
Fixed in 94a7043. A subclass that explicitly opts in with its own group now retains the constraints from every explicitly opted-in MRO owner, so it is rejected against competitors in both its inherited group and its own group before discovered factories run. None cannot erase an inherited constraint, and duplicate inherited groups count only once per plugin registration.
The closest-marker gate is unchanged: an invalid closest marker still makes the class inert. Unmarked legacy group fields remain ignored; callback ownership/binding and the one-callback-per-plugin lifecycle are unchanged.
Added 18 core cases covering both groups, multiple inheritance, repeated groups, None, invalid markers and legacy fields, plus 12 real OTel cases covering both views through explicit, environment and mixed registration. Fourteen regression cases failed with the previous implementation. All 1,765 core tests (+5 subtests, 98.28% coverage), 356 OTel tests and 14 installed-core 2.0.1 compatibility cases pass, along with type, lint/format and commit checks. The READMEs and PR description now describe additive groups.
There was a problem hiding this comment.
Follow-up: the same subclass-owned group masking was reproduced through real registration in JS #956 and fixed in 8807415. Explicitly opted-in static registration groups now accumulate along the constructor inheritance chain and are deduplicated per registration. A subclass's own group therefore retains the inherited OTel view constraint. Concrete getter receivers and the existing static/legacy-instance fallback behavior are preserved, and conflicts are rejected before any factory runs.
Five new regression cases failed with the previous runtime. Local validation passed 1,301 core tests and 18 real OTel registration/installed-compatibility tests, plus the core/OTel builds, type checks and repository lint. The fix remains included in the latest published base-sync head 4dfe430.
| 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 or dispatch mechanism if every pending run must be retained.
Codex AI reviewOne P1 workflow-validation issue remains; no additional actionable defects were found in the SDK/OTel changes. 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 gates registration and supplies the cleanup callback; an explicit marker other than integer1still shadows inherited opt-in. Once enabled, every class explicitly declaring integer1contributes its resolved group. A subclass's new group adds to inherited groups, andNonecannot erase an inherited constraint. Repeated groups count once per plugin registration. Unmarked legacy fields/helpers remain inert, callback ownership and binding stay unchanged, and the callback still runs once per plugin. 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: