Skip to content

Conversation

@mabdinur
Copy link
Contributor

@mabdinur mabdinur commented Nov 4, 2022

Description

Resolve: #4397

In Starlette and FastAPI applications the last middleware configured is the first one called (here). The fastapi trace middleware is applied when a fastapi application is created (fastapi.FastAPI()). If another middleware is configured after an application is initialized it will not be traced.

Here's an example of a fastapi middleware being configured after fastapi.FastApi() is called: https://fastapi.tiangolo.com/tutorial/middleware/#create-a-middleware.

Note - This PR only ensures Fastapi middlewares are traced. Supporting all starlette middlewares is out of scope.

Reviewer Checklist

  • Title is accurate.
  • Description motivates each change.
  • No unnecessary changes were introduced in this PR.
  • Avoid breaking API changes unless absolutely necessary.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Release note has been added for fixes and features, or else changelog/no-changelog label added.
  • All relevant GitHub issues are correctly linked.
  • Backports are identified and tagged with Mergifyio.

@mabdinur mabdinur force-pushed the fastapi/fix-tracing-in-middlewares branch 2 times, most recently from a07a1f8 to 95ff33e Compare November 4, 2022 18:40
@mabdinur mabdinur changed the title chore(fastapi): add reproduction for fastapi bug fix(fastapi): apply tracing to fastapi middleware Nov 4, 2022
@mabdinur mabdinur force-pushed the fastapi/fix-tracing-in-middlewares branch from 95ff33e to b9f8cad Compare November 7, 2022 14:50
@mabdinur mabdinur marked this pull request as ready for review November 21, 2022 19:33
@mabdinur mabdinur requested a review from a team as a code owner November 21, 2022 19:33
@mabdinur mabdinur changed the title fix(fastapi): apply tracing to fastapi middleware fix(fastapi): apply tracing to fastapi middlewares Nov 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2022

Codecov Report

Merging #4478 (413520c) into 1.x (68fad0d) will decrease coverage by 0.00%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##              1.x    #4478      +/-   ##
==========================================
- Coverage   74.62%   74.61%   -0.01%     
==========================================
  Files         811      811              
  Lines       62335    62357      +22     
==========================================
+ Hits        46515    46527      +12     
- Misses      15820    15830      +10     
Impacted Files Coverage Δ
ddtrace/contrib/fastapi/patch.py 88.13% <83.33%> (-0.55%) ⬇️
tests/contrib/fastapi/test_fastapi.py 100.00% <100.00%> (ø)
ddtrace/internal/remoteconfig/worker.py 43.47% <0.00%> (-4.35%) ⬇️
ddtrace/internal/writer.py 56.96% <0.00%> (-2.17%) ⬇️
ddtrace/internal/telemetry/writer.py 33.79% <0.00%> (-0.69%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mabdinur mabdinur changed the title fix(fastapi): apply tracing to fastapi middlewares fix(fastapi): apply tracing to user middlewares in fastapi Nov 23, 2022
@mabdinur mabdinur force-pushed the fastapi/fix-tracing-in-middlewares branch from f8f8da6 to 68cd3fa Compare December 22, 2022 21:58
@mabdinur mabdinur requested a review from ZStriker19 December 22, 2022 21:58
ZStriker19
ZStriker19 previously approved these changes Dec 22, 2022
@mabdinur mabdinur enabled auto-merge (squash) December 22, 2022 22:24
@mabdinur mabdinur requested review from Yun-Kim and ZStriker19 January 9, 2023 16:35
@mabdinur mabdinur requested a review from a team as a code owner February 2, 2023 14:59
emmettbutler
emmettbutler previously approved these changes Feb 2, 2023
@DataDog DataDog deleted a comment from mergify bot Feb 2, 2023
ZStriker19
ZStriker19 previously approved these changes Feb 6, 2023
@mabdinur mabdinur merged commit b5a4511 into 1.x Feb 9, 2023
@mabdinur mabdinur deleted the fastapi/fix-tracing-in-middlewares branch February 9, 2023 04:34
@github-actions github-actions bot added this to the v1.9.0 milestone Feb 9, 2023
majorgreys pushed a commit that referenced this pull request Feb 10, 2023
## Description

Resolve: #4397

In Starlette and FastAPI applications the last middleware configured is
the first one called
([here](https://github.com/encode/starlette/blob/0.21.0/starlette/applications.py#L168)).
The fastapi trace middleware is applied when a fastapi application is
created
([fastapi.FastAPI()](https://github.com/DataDog/dd-trace-py/blob/a21c8dbc433577245f5c145fd46f2a3dc4540402/ddtrace/contrib/fastapi/patch.py#L38)).
If another middleware is configured after an application is initialized
it will not be traced.

Here's an example of a fastapi middleware being configured after
`fastapi.FastApi()` is called:
https://fastapi.tiangolo.com/tutorial/middleware/#create-a-middleware.

Note - This PR only ensures Fastapi middlewares are traced. Supporting
all starlette middlewares is out of scope.

## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] Avoid breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is
included in the code or PR.
- [ ] Release note has been added for fixes and features, or else
`changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.

Co-authored-by: Brett Langdon <[email protected]>
(cherry picked from commit b5a4511)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trace middleware in Fastapi

7 participants