chore(serverless): publish web request-start event - #19898
litianningdatadog wants to merge 1 commit into
Conversation
Codeowners resolved asResolved from the full PR diff against |
Circular import analysis
|
Dependency direction analysis
|
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 1 Pipeline job failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 1e76b73 | Docs | View more details | Give us feedback! |
litianningdatadog
changed the title
4aebb33 to
206cd22
Compare
206cd22 to
f3ea773
Compare
f3ea773 to
68bf61e
Compare
68bf61e to
8e26c22
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The environment detector lacks direct coverage, and the release note announces behavior completed only by a follow-up PR.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds pre-span MicroVM request detection to WSGI and ASGI instrumentation, supporting the runtime identity refresh stack.
Changes:
- Dispatches
web.request.startingfor matching MicroVM requests. - Adds explicit runtime identity refresh callbacks while preserving fork lineage.
- Adds focused integration/runtime tests and a release note.
File summaries
| File | Description |
|---|---|
ddtrace/contrib/_events/web_framework.py |
Defines the request-start event. |
ddtrace/contrib/internal/web.py |
Matches and dispatches MicroVM requests. |
ddtrace/contrib/internal/asgi/middleware.py |
Dispatches before ASGI span creation. |
ddtrace/contrib/internal/wsgi/wsgi.py |
Dispatches before WSGI span creation. |
ddtrace/internal/serverless/__init__.py |
Detects the MicroVM environment. |
ddtrace/internal/runtime/__init__.py |
Adds explicit identity refresh support. |
tests/contrib/asgi/test_asgi.py |
Tests ASGI matching and ordering. |
tests/contrib/wsgi/test_wsgi.py |
Tests WSGI request matching. |
tests/tracer/runtime/test_runtime_id.py |
Tests refresh and fork semantics. |
releasenotes/notes/add-lambda-microvm-web-request-starting-ffa6841a15e3c57c.yaml |
Announces MicroVM request detection. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
8e26c22 to
cdc53d4
Compare
d28bc4e to
0d5cf10
Compare
litianningdatadog
added
the
changelog/no-changelog
0d5cf10 to
4654514
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24e4d34d84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
24e4d34 to
2d71731
Compare
jcstorms1
left a comment
jcstorms1
left a comment
There was a problem hiding this comment.
Just looked at the files we own. LGTM
2d71731 to
bfe3d91
Compare
bfe3d91 to
a44c5ce
Compare
a44c5ce to
7398b18
Compare
Stacked PRs: - 👉 This #19778 — runtime identity foundation - #19898 — WSGI/ASGI request-start hook - #19939 — central MicroVM /run identity refresh - #19820 — tracer/native writer exporter refresh - #19821 — telemetry worker refresh - #19822 — runtime metrics runtime-id tags ## Description Adds `ddtrace.internal.runtime.refresh_identity()` for runtimes that need a fresh runtime ID without going through an OS fork. This introduces `on_runtime_identity_refresh()`, a dedicated callback registry for consumers that should react only to explicit identity refreshes. Existing `on_runtime_id_change()` behavior remains available for fork-related and general runtime-ID change handling. `refresh_identity()` rotates the runtime ID without recording parent or ancestor lineage because a resumed MicroVM run is not a real fork. Existing fork behavior and lineage tracking remain unchanged. Refresh callbacks are isolated by default so one failed consumer does not block others. Callers coordinating a refresh transaction can opt into error propagation with `raise_on_error=True` and retry the same identity refresh when a rebuild fails. ## Reference - [RFC](https://docs.google.com/document/d/17lde4Zak2YRBuDvf32KFanXj9TdpOv24GYaEY8swA1w/edit?tab=t.0#heading=h.wwu37decnk8j) ## Testing Added coverage for: - runtime ID rotation through `refresh_identity()` - preservation of parent and ancestor fork lineage - explicit refresh subscriber notification - refresh subscribers not being invoked by fork handling - callback failure isolation by default - optional refresh callback failure propagation - successful callbacks under `raise_on_error=True` Validation: - `scripts/run-tests -s --venv 190fcc7 -- -q tests/tracer/runtime/test_runtime_id.py` — 21 passed - `scripts/lint checks` — passed ## Risks Low. The new path is opt-in and is not wired to traffic in this PR. Existing fork behavior is preserved. Co-authored-by: tianning.li <tianning.li@datadoghq.com>
lym953
left a comment
lym953
left a comment
There was a problem hiding this comment.
Approving blindly as I don't have knowledge on any of the files, and since apm-serverless has approved it.
Will discuss whether we should remove serverless-aws as owner.
7398b18 to
6e98bbd
Compare
quinna-h
left a comment
quinna-h
left a comment
There was a problem hiding this comment.
LGTM overall for the test files that IDM owns; left one comment about a test that was supposed to prove the request-start event happens before the request span is created, but instead the span marker records when the middleware retrieves the span
6e98bbd to
a251b4d
Compare
| request.META.get("SCRIPT_NAME") or "", | ||
| request.META.get("PATH_INFO") or "", | ||
| ): | ||
| request.META[_WEB_REQUEST_STARTING_DISPATCHED] = True |
There was a problem hiding this comment.
comment could be nice here
| environ.get("SCRIPT_NAME") or "", | ||
| environ.get("PATH_INFO") or "", | ||
| ): | ||
| environ[_WEB_REQUEST_STARTING_DISPATCHED] = True |
There was a problem hiding this comment.
same here, comment with reasoning would be good.
|
|
||
| path = path_prefix.rstrip("/") + path | ||
| if path != _LAMBDA_MICROVM_RUN_PATH: | ||
| return False |
There was a problem hiding this comment.
This logic should be moved to the microvm listener/runtime id specific listener of the event.
doing the method and path checking for microvm run path in the event dispatcher isn't really any different/better than just hard coding it into each integration, we want to remove product specific behaviors/code from the integrations and dispatch them through the event hub.
so this should be something more like:
def dispatch_web_request_starting(method: Optional[str], path_prefix: str, path: str) -> None:
core.dispatch(WebFrameworkEvents.WEB_REQUEST_STARTING.value, (method, path))
def check_microvm_run(method: Optional[str], path_prefix: str):
if method != "POST":
return
path = path_prefix.rstrip("/") + path
if path != _LAMBDA_MICROVM_RUN_PATH:
return
# trigger behaviors we want when this endpoint is called by microvm
if in_aws_lambda_microvm():
core.on(WebFrameworkEvents.WEB_REQUEST_STARTING.value, check_microvm_run)what we are deciding is that ASGI/WSGI integrations should emit an event when the web request is starting, and then allowing different products/code to then use those events to perform different actions (in our case, when in a microvm we should refresh identity information)
There was a problem hiding this comment.
Agreed. I will revise PR #19898 & #19939 as #19898 now publishes only the generic web.request.starting event with the request method and normalized path. It no longer contains MicroVM environment, method, or path checks.
#19939 registers the runtime-ID listener only in MicroVM processes and owns the exact POST /aws/lambda-microvms/runtime/v1/run match and identity refresh. This keeps product behavior out of the WSGI/ASGI integrations. Let me know whether that makes sense.
a251b4d to
3d8d4ca
Compare
litianningdatadog
changed the title
dubloom
left a comment
dubloom
left a comment
There was a problem hiding this comment.
Some small comments but overall LGTM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11ab2d5fe7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not environ.get(_WEB_REQUEST_STARTING_DISPATCHED): | ||
| if trace_utils.dispatch_web_request_starting( |
There was a problem hiding this comment.
In an AWS Lambda MicroVM running an automatically instrumented Falcon app without manually adding DDWSGIMiddleware, this code is never reached: a repo-wide search shows that ddtrace/contrib/internal/falcon/patch.py:40-48 installs Falcon's separate falcon.middleware.TraceMiddleware, whose process_request creates the root request context directly at ddtrace/contrib/internal/falcon/middleware.py:34-56. The new Django-specific hook resolves the earlier Django case, but Falcon—and similarly structured WSGI integrations—still emits no start event, so the first request span after a MicroVM resume retains the previous runtime identity. Publish the event from these automatically installed WSGI instrumentation paths or introduce a shared hook they invoke.
AGENTS.md reference: AGENTS.md:L60-L63
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Scoping this PR to the generic WSGI/ASGI middleware and Django. Falcon, along with Bottle, Pyramid, CherryPy, and Molten (which also create the root span without going through DDWSGIMiddleware), will get request-start coverage in a follow-up PR. The scope is stated above the helpers in trace_utils.py and in the PR description.
| trace_utils.dispatch_web_request_starting( | ||
| method, | ||
| scope.get("root_path") or "", | ||
| scope.get("path") or "", | ||
| ) |
There was a problem hiding this comment.
For ASGI scopes where path already contains root_path, as occurs with mounted or server-prefixed applications, this unconditionally constructs a duplicated path such as /aws/lambda-microvms/runtime/v1/aws/lambda-microvms/runtime/v1/run. The same middleware later treats scope["path"] as full_path and uses it directly when constructing the request URL at lines 323-337, so callers cannot be assumed always to provide only the post-prefix remainder used by the new test. Because the MicroVM listener matches the runtime /run endpoint, these requests silently miss the refresh event; normalize based on whether path already includes the prefix before concatenating.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1e76b73. dispatch_asgi_web_request_starting now prepends root_path only when path does not already start with it on a path-segment boundary (ignoring a trailing slash on root_path), the same rule as Starlette's get_route_path. Added a test case where path already includes root_path.
| # Django's automatic WSGI instrumentation bypasses DDWSGIMiddleware, but an | ||
| # application can still wrap it. The shared environ marker lets the first | ||
| # layer publish the request-start event before either layer creates a span. | ||
| if request.META.get("wsgi.version") is not None and not request.META.get(_WEB_REQUEST_STARTING_DISPATCHED): |
There was a problem hiding this comment.
In the supported Django 3.0 ASGI configuration, requests reach the synchronous traced_get_response path but their META is built from an ASGI scope and has no wsgi.version, so this condition suppresses the event. The fallback ASGI middleware does not cover this version: ddtrace/contrib/internal/django/patch.py:471-477 deliberately wraps get_asgi_application only for Django 3.1+, while the test matrix and test_asgi_200 explicitly exercise Django 3.0 ASGI. Consequently a Django 3.0 application served through Daphne in a MicroVM creates its request span without refreshing runtime identity; add an ASGI-aware dispatch path for the pre-3.1 handler.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1e76b73. Dropped the wsgi.version guard: the hook now calls dispatch_wsgi_web_request_starting(request.META) for every request. Django 3.0's ASGIRequest.META carries SCRIPT_NAME/PATH_INFO with root_path already stripped, so the path comes out right. Django 3.1+ ASGI requests go through get_response_async and the ASGI middleware instead, so there is no double dispatch. Added a test that builds a real ASGIRequest; it passes on Django 3.0 and 5.1.
…plications Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Stacked PRs:
Description
This PR provides the framework side of the MicroVM identity-refresh flow. WSGI,
ASGI, and Django publish
web.request.startingbefore they create a root span,passing the request method and normalized path.
The publisher is deliberately product-neutral: it does not import MicroVM
helpers, recognize the MicroVM
/runendpoint, or decide whether to refresh aruntime ID. PR #19939 registers the MicroVM-only listener and owns that policy.
When no product listens for the event, the publisher returns after a single
listener check, before reading the request or normalizing the path.
Two helpers in
trace_utilsown the publishing:dispatch_wsgi_web_request_starting(environ)is used byDDWSGIMiddlewareand Django's
get_responsehook. WSGI and Django share an environ marker sonested layers publish one event. The Django hook also covers Django 3.0 ASGI
requests, which reach the sync
get_responsebecauseget_asgi_applicationis only wrapped on Django 3.1+.
dispatch_asgi_web_request_starting(scope)publishes only from the outer ASGIapplication. It accepts scopes whether or not
pathalready includesroot_path, so mounted or server-prefixed apps do not produce a duplicatedprefix.
Scope: the event is published for the generic WSGI/ASGI middleware and Django.
Frameworks whose automatic instrumentation creates the root span without
DDWSGIMiddleware(Falcon, Bottle, Pyramid, CherryPy, Molten) are out of scopefor this PR and will follow separately.
This implements the requested split in review comment r4146791727.
Reference
Testing
scripts/lint fmtandscripts/lint typingon the changed filesDjango 5.1 (py3.13), including a real
ASGIRequestreachingget_responseRisks
There is no public API change. The only production listener in this stack is
registered by PR #19939 in MicroVM processes, so non-MicroVM requests do not
dispatch this event.
🤖 Generated with Claude Code