fix(starlette): fixes duplication due to changes path behavior w/latest starlette versions
#8577
Conversation
zarirhamza
changed the title
root_path and path behavior with latest starlette versionspath behavior w/latest starlette versions
Datadog ReportBranch report: ✅ 0 Failed, 171709 Passed, 1037 Skipped, 11m 7.1s Wall Time |
| @@ -85,6 +85,17 @@ def span_from_scope(scope: Mapping[str, Any]) -> Optional[Span]: | |||
| return scope.get("datadog", {}).get("request_spans", [None])[0] | |||
|
|
|||
|
|
|||
| def _extract_full_path(raw_path, root_path, path): | |||
| if root_path and raw_path and raw_path.startswith(root_path.encode()): | |||
| raw = raw_path.split(b"?")[0] if b"?" in raw_path else raw_path | |||
There was a problem hiding this comment.
Is the raw path guaranteed to be a byte string? Can we use urlparse here? Removing the query args using the ? seems a bit brittle
| if raw.decode() != check_path: | ||
| return path | ||
|
|
||
| return root_path + path |
There was a problem hiding this comment.
| return root_path + path | |
| return check_path |
| @@ -85,6 +85,17 @@ def span_from_scope(scope: Mapping[str, Any]) -> Optional[Span]: | |||
| return scope.get("datadog", {}).get("request_spans", [None])[0] | |||
|
|
|||
|
|
|||
| def _extract_full_path(raw_path, root_path, path): | |||
There was a problem hiding this comment.
Can we include a doc string describing the new behavior?
| "36": parse_version(sys.version) < (3, 7), # 3.6 has an extra request | ||
| "rest": parse_version(sys.version) >= (3, 7), | ||
| "old": starlette_version < (0, 20, 0), # below 0.20 has an extra request due to python 3.6 support | ||
| "new": starlette_version >= (0, 20, 0), |
There was a problem hiding this comment.
There seems to be two changes here:
- Updating the riofile to run starlette tests with
<v.20.0 - Fixing a bug in how asgi middlewares set the http.url tag
Can we split up these changes?
Also would it be possible to avoid running snapshot tests on older versions of starlette? I think we have sufficient test coverage already.
| @@ -542,8 +540,8 @@ def test_background_task(snapshot_client_with_tracer, tracer, test_spans): | |||
| ) | |||
| @pytest.mark.snapshot( | |||
| variants={ | |||
| "36": parse_version(sys.version) < (3, 7), # 3.6 has an extra request | |||
| "rest": parse_version(sys.version) >= (3, 7), | |||
| "old": starlette_version < (0, 20, 0), # below 0.20 has an extra request due to python 3.6 support | |||
There was a problem hiding this comment.
Can we expand on this comment. It is not immediately clear why starlette_version<0.20 needs a unique snapshot file.
root_pathnow contains duplicated info of the mounted path found inpathforstarlette> v0.34.Previously, there was no duplication when mounting applications on
starlette. Logic is now implemented in the asgi TraceMiddleware to check theroot_pathandpathvs theraw_pathto determine duplication.Checklist
changelog/no-changelogis set@DataDog/apm-tees.@DataDog/security-design-and-guidance.Reviewer Checklist