[vastin]

Description of changes

This PR adds the ServiceEvents feature to the ADOT Python distro: a set of
higher-level telemetry signals derived from function-level and HTTP endpoint
activity, emitted via OTLP.

ServiceEvents is opt-in and bundled-gated behind Application Signals
(OTEL_AWS_SERVICE_EVENTS_ENABLED / OTEL_AWS_APPLICATION_SIGNALS_ENABLED); it
is disabled in AWS Lambda.

Summary

When active, ServiceEvents emits function-call duration metrics, HTTP endpoint
summaries, per-exception error counts, deployment events, and incident snapshots.
Function-level instrumentation is scoped by an explicit allowlist, and endpoint
tracking is wired through framework middleware for Flask, FastAPI, and Django.

What's included

• ServiceEvents package and configurator wiring to initialize it when enabled.
• Unit tests for the package.
• Contract tests with sample apps and mock-collector infrastructure.

Testing

• Unit and contract test suites pass.
• tox -e lint and tox -e spellcheck pass.

By submitting this pull request, I confirm that my contribution is made under

[github-actions]

Claude finished @vastin's task in 4m 27s —— View job

---

Reviewing PR #763

• Check for prior reviews to avoid duplicates
• Read CLAUDE.md for repo conventions
• Review the full PR diff (core modules)
• Analyze findings
• Post summary

---

Code Review Summary

This is a well-structured, large feature addition (~21k lines) with good crash-safety discipline. The never_raises pattern, broad-exception-caught guards on every customer-facing path, and clear separation between telemetry and app code are commendable. Here are my substantive findings:

---

1. Region extraction is fragile (low severity)

File: serviceevents_instrumentation.py:62

region = (logs_endpoint or "").lower().split(".")[1]

This extracts the region by splitting on dots and taking index [1]. It's protected by the re.match guard on line 57 that guarantees the format, but it's brittle to future refactoring — if anyone changes the regex or reorders the guard, this becomes an IndexError. The regex already captures the region in group 1. Consider:

match = re.match(AWS_LOGS_OTLP_ENDPOINT_PATTERN, (logs_endpoint or "").lower())
is_cw_endpoint = bool(match)
...
region = match.group(1)

---

2. Traceback reference retention in investigation data (medium severity — memory)

File: python_monitor_impl.py:523

"traceback_info": (exc_type, exc_value, exc_traceback),

Storing a reference to exc_traceback keeps the entire frame chain alive (all local variables in every frame) until the investigation data is cleared. When no incident is triggered (the common case for non-500 responses), the data persists in the ContextVar until the next request on that context calls begin_investigation(). This creates a GC-pinning window that can be large under low request rates.

Consider formatting the traceback string immediately and storing only the string, or explicitly del-ing the traceback reference after storing it:

"traceback_info": None,  # formatted eagerly below
"stack_trace": "".join(traceback.format_exception(exc_type, exc_value, exc_traceback)),

---

3. _call_counters grows unboundedly in auto sampling mode (medium severity — memory)

File: python_monitor_impl.py:30-97

_call_counters: Dict[str, int] = {}

In auto mode, _increment_call_counter adds one entry per unique function name and never evicts. For a service with many instrumented functions running for days, this dict grows monotonically. The counters also overflow int given enough time (less of a concern in Python). Consider either periodic reset (e.g., at each collector flush), or switching to a bounded structure once past tier3.

---

4. begin_investigation() is called unconditionally for every request (low severity — performance)

File: flask_instrumentation.py:156

monitor_state.begin_investigation()

This allocates a new dict (with a list and a time.time() call) on every single request, even healthy ones that will never trigger an incident. For very high-throughput services, this adds measurable allocation pressure. Consider lazily initializing investigation data only when an error is actually detected.

---

5. Missing explicit cleanup of investigation data on non-incident path

Files: flask_instrumentation.py, fastapi_instrumentation.py, django_instrumentation.py

When process_potential_incident returns None (no incident triggered), get_investigation_data() (which clears the ContextVar) is never called. The investigation data — including any stored traceback references from finding #2 — lingers in the ContextVar until the next request's begin_investigation() overwrites it. Adding an explicit clear in the finally block of teardown would be cleaner:

finally:
    clear_current_operation()
    monitor_state.get_investigation_data()  # Clear leftover investigation data

---

6. DEBUG_SE_TEST_CONFIG env var provides a hidden config surface (informational)

File: config.py:83-139

The _apply_test_config_hook function allows overriding internal config fields via DEBUG_SE_TEST_CONFIG. While documented as test-only and gated behind a warning, this is a runtime-accessible hook that can modify flush intervals and sampling thresholds in production. The one-time warning is appropriate, but consider also gating this on a debug/non-production build flag or at minimum logging at ERROR level.

---

7. Hardcoded SDK version "0.14.2" in default parameters

Files: incident_snapshot_collector.py:71, endpoint_collector.py:44

sdk_version: str = "0.14.2",

These default values appear stale — the current SDK version is fetched from ADOT_VERSION in the config module. The defaults are overridden by ServiceEventsConfig.sdk_version at construction time, so they're dead code in practice, but they're confusing to readers. Consider using "" or removing the default entirely to force explicit passing.

---

Overall Assessment

The code is production-quality with excellent crash-safety discipline. The architecture (AST hooks + periodic collectors + OTLP emission) is sound. The findings above are mostly about memory hygiene under sustained load rather than correctness bugs. The test coverage at 21k+ lines (with unit + contract tests) is comprehensive.

Key strengths:

• Consistent never_raises / broad-except guards prevent telemetry from crashing customer apps
• Clean singleton lifecycle with proper fork handling
• Good use of ContextVars for async/threading safety
• SDK self-exclusion (SDK_SELF_EXCLUDE) prevents infinite recursion
• Generator detection prevents incorrect instrumentation of generators/async generators

[github-actions]

Claude finished @vastin's task in 7m 30s —— View job

---

Reviewing PR #763 (post-sync)

• Check for prior reviews to avoid duplicates
• Read CLAUDE.md for repo conventions
• Review the updated PR diff
• Analyze findings and deduplicate
• Post summary

---

Code Review — Post-Sync Analysis

This review focuses on the current state of the feat/service-events branch (commit 48e592e). The previous comment already covered the high-level architecture favorably; this pass focuses on specific issues that remain actionable.

---

1. Traceback reference retention in inv_data["exception"] (medium — memory)

File: python_monitor_impl.py:523

"traceback_info": (exc_type, exc_value, exc_traceback),

Storing exc_traceback pins the entire frame chain (all locals in every frame) until get_investigation_data() clears the ContextVar. On the non-incident path (the common case — most requests are healthy), this data lingers until the next request's begin_investigation() overwrites it. Under low request rates that GC-pinning window can be large.

Suggestion: Format the traceback eagerly and store only the string, or explicitly del exc_traceback after storing. The _collect_exception_info in incident_snapshot_collector.py already handles the formatting gracefully.

---

2. self.exception_info is dead code (low — clarity)

File: python_monitor_impl.py:474-479

self.exception_info = {
    "type": exc_type,
    "value": exc_value,
    "traceback": exc_traceback,
}

This attribute is set in __exit__ but never read by any downstream consumer. Exception data propagation happens exclusively through inv_data["exception"] (line 490). The dead assignment also holds a redundant reference to the traceback. Consider removing it.

---

3. Region extraction is fragile (low — maintainability)

File: serviceevents_instrumentation.py:62

region = (logs_endpoint or "").lower().split(".")[1]

The regex guard on line 56 guarantees the format, but the extraction relies on positional splitting rather than using the regex's capture group. If the regex or guard is ever refactored, this becomes an IndexError. Safer:

match = re.match(AWS_LOGS_OTLP_ENDPOINT_PATTERN, (logs_endpoint or "").lower())
region = match.group(1)  # regex already captures region

---

4. Stale "0.14.2" version defaults in collector constructors (low — clarity)

Files: incident_snapshot_collector.py:70, endpoint_collector.py:43, ast_transformation.py:163

These default parameter values are never exercised in production — ServiceEventsConfig.sdk_version (which reads from ADOT_VERSION) is always passed explicitly. The hardcoded "0.14.2" is confusing to readers. Consider using "" to signal "must be provided" or importing ADOT_VERSION directly.

---

5. Investigation data not explicitly cleared on non-incident path (low — hygiene)

Files: flask_instrumentation.py, fastapi_instrumentation.py, django_instrumentation.py

When process_potential_incident returns None (no incident), get_investigation_data() is never called for that request. The data (including any traceback_info from finding #1) lingers in the ContextVar until the next request's begin_investigation() overwrites it. Adding an explicit clear in the finally block would make the lifecycle predictable:

finally:
    clear_current_operation()
    monitor_state.get_investigation_data()  # Clear stale investigation data

This combines with finding #1 — if traceback references are formatted eagerly, the lingering data is just a small dict, making this lower priority.

---

6. _call_counters grows unboundedly in auto mode (medium — memory)

File: python_monitor_impl.py:30

_call_counters: Dict[str, int] = {}

In auto sampling mode, every unique function name gets an entry that is never evicted. For a long-lived service with many instrumented functions, this grows monotonically. The __enter__ optimization (line 446) correctly skips the counter for non-auto modes, so this only affects auto. Consider periodic reset (e.g., at each collector flush cycle) or a bounded LRU.

---

Overall Assessment

The code is production-quality with excellent crash-safety discipline throughout. All findings from the initial review remain applicable in this sync — they are all "nice to have" improvements rather than blockers. The architecture (AST hooks + periodic collectors + OTLP emission) is sound, the fork-safety handling is thorough, and the test coverage is comprehensive.

[github-actions]

Claude finished @vastin's task in 6m 27s —— View job

---

Reviewing PR #763 (post-sync)

• Check for prior reviews to avoid duplicates
• Read CLAUDE.md for repo conventions
• Review the full PR diff (core modules)
• Deduplicate findings against prior reviews
• Post review summary

---

Code Review — Post-Fix Analysis (commit f6ab442)

The fix commit addressed the most impactful findings from earlier reviews: traceback retention (memory), dead self.exception_info, stale "0.14.2" defaults, Django double-init, FastAPI constructor guard, and AST docstring preservation. All were verified fixed.

Remaining previously-reported issues (still applicable, not re-posted):

• _call_counters unbounded growth in auto mode (medium — memory)
• Region extraction via split(".")[1] instead of regex capture group (low — maintainability)
• Investigation data not explicitly cleared on non-incident path (low — mitigated by the traceback fix)

New finding:

FastAPI middleware: unguarded error-response send can suppress the original exception

File: fastapi_instrumentation.py:271-285

except Exception as exc:
    exception = exc
    status_code = 500

    # If response hasn't started, send a 500 error response
    if not response_started:
        await send(...)  # <-- unguarded
        await send(...)  # <-- unguarded

    # Re-raise to maintain error propagation
    raise

If send() raises (client disconnect, broken pipe, ASGI server shutdown), the new exception propagates instead of the original application exception — raise on line 288 never executes. This changes error semantics for any downstream exception handler expecting the original error. The telemetry-must-never-alter-control-flow principle applies here: this code path alters which exception the application sees.

Suggested fix — guard the error-response send so the original exception always propagates:

except Exception as exc:
    exception = exc
    status_code = 500

    if not response_started:
        try:
            await send({"type": "http.response.start", "status": 500, ...})
            await send({"type": "http.response.body", "body": b"Internal Server Error"})
        except Exception:  # pylint: disable=broad-exception-caught
            pass  # Best-effort error response; original exception still re-raises

    raise

Severity: Low-medium. Only triggers on concurrent client disconnect during an application error — unlikely in practice but violates the stated crash-safety invariant.

---

Overall Assessment

Excellent production-quality code with comprehensive crash-safety discipline. The fix commit addressed all the critical findings from earlier reviews. The single new finding above is a minor robustness gap — the PR is in good shape for merge.

[jj22ee]

Added some comments generated through Claude based on AI review.

[github-actions]

Claude finished @vastin's task in 3m 58s —— View job

---

Reviewing PR #763 (post-sync)

• Check for prior reviews to avoid duplicates
• Read CLAUDE.md for repo conventions
• Review the full PR diff
• Deduplicate findings against prior reviews
• Post review summary

---

Code Review — Post-Fix Analysis (commit f0713e9)

The fix commit thoroughly addressed the findings from @jj22ee's review:

• Investigation ContextVar leak → clear_investigation_data() added, called unconditionally in every framework's finally path
• AST docstring preservation → _preamble_insert_index inserts after module docstring + __future__ imports
• Flask/FastAPI init crash-guard → original_init runs outside the try; only hook registration is guarded
• Django settings.MIDDLEWARE → Temporarily prepends for the single load_middleware build, then restores
• Unmatched route cardinality → Shared UNMATCHED_ROUTE sentinel in _constants.py across all three frameworks
• Stale version defaults → Changed to "" in collector constructors

All fixes are well-implemented with appropriate test coverage.

One new finding posted as an inline comment:

The FastAPI error handler's await send() calls (lines 275-287) are not try/excepted — if send() raises (client disconnect during an app error), it suppresses the original exception. This violates the telemetry-must-never-alter-control-flow invariant stated throughout the codebase. Low-medium severity; see the inline comment for a suggested fix.

Previously-reported issues still applicable (not re-posted — already noted in prior automated reviews):

• _call_counters unbounded growth in auto sampling mode (medium — memory, python_monitor_impl.py:31)
• Region extraction via split(".")[1] instead of regex capture group (low — maintainability, serviceevents_instrumentation.py:83)

Overall: Excellent production-quality code. The fix commit demonstrates strong attention to the crash-safety invariant. The PR is in good shape.

[jj22ee]

Apart from the bot's concern regarding unintended suppression of original application exception, lgtm from my side

[github-actions]

Claude finished @vastin's task in 2m 13s —— View job

---

Reviewing PR #763 (post-sync)

• Check for prior reviews to avoid duplicates
• Read CLAUDE.md for repo conventions
• Review the latest changes (commit 013e993)
• Deduplicate findings against prior reviews
• Post review summary

---

Code Review — Post-Fix Analysis (commit 013e993)

The latest commit addresses the one remaining actionable finding from my prior inline review: the unguarded await send() in the FastAPI error handler that could suppress the original application exception.

Fix verified: The error-response send() calls (lines 281–293) are now wrapped in try/except Exception: pass, ensuring the original application exception always propagates via the bare raise on line 298. A targeted test (test_middleware_send_failure_does_not_mask_app_exception) confirms that an OSError from send() does not mask the app's RuntimeError.

Previously-reported issues still applicable (not re-posted — already noted in prior automated reviews):

• _call_counters unbounded growth in auto sampling mode (medium — memory, python_monitor_impl.py:31)
• Region extraction via split(".")[1] instead of regex capture group (low — maintainability, serviceevents_instrumentation.py:83)

No new findings. All substantive issues from prior reviews have been addressed. The PR is in good shape for merge.

---