Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix:Update starlette.asciidoc, incorrect middleware order #1956

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

mariocandela
Copy link
Contributor

What does this pull request do?

The documentation contains an error, you need to put the APM middleware before BaseHTTPMiddleware.

@github-actions github-actions bot added agent-python community Issues opened by the community triage Issues awaiting triage labels Jan 23, 2024
@basepi
Copy link
Contributor

basepi commented Jan 23, 2024

I don't think this is correct:

Using BaseHTTPMiddleware will prevent changes to contextlib.ContextVars from propagating upwards.

So, BaseHTTPMiddleware needs to be above (i.e. before) our middleware in the list so that we have an unbroken contextvar chain from the endpoint to our middleware.

Does this not match your experience?

Edit: see also the original issue: #1701

@mariocandela
Copy link
Contributor Author

mariocandela commented Jan 24, 2024

I don't think this is correct:

Using BaseHTTPMiddleware will prevent changes to contextlib.ContextVars from propagating upwards.

So, BaseHTTPMiddleware needs to be above (i.e. before) our middleware in the list so that we have an unbroken contextvar chain from the endpoint to our middleware.

Does this not match your experience?

Edit: see also the original issue: #1701

Yesterday, we integrated the APM agent across all our microservices, placing the agent's middleware as the last element. No data was reaching the APM server in this configuration. However, when we placed it as the first element, everything worked correctly. Could it be that one of our custom middlewares(BaseHTTPMiddleware) was invalidating the context variable?

Thanks for your time Colton 🥇

@basepi
Copy link
Contributor

basepi commented Jan 26, 2024

Are you passing in the middleware as a list or using the add_middleware() function repeatedly? I think that may be where the confusion is -- the original issue #1701 was using add_middleware, which appears to insert the middleware at the front of the list. Thus, the required order would be swapped based on how you're adding the middleware.

I think we need to expand this warning with examples of both.

@v1v
Copy link
Member

v1v commented Feb 8, 2024

run docs-build

@basepi
Copy link
Contributor

basepi commented Feb 13, 2024

@mariocandela Can you answer my question above?

@mariocandela
Copy link
Contributor Author

@mariocandela Can you answer my question above?

Hi @basepi, sorry for the delay, we use middleware as a list.
Initially, we had the agent's middleware placed last, but data wasn't reaching the APM server in this setup. However, when we moved it to the first position, everything started working correctly.

I haven't had the chance to delve deeper into the issue yet. It's possible that our custom middleware, which utilizes dependency injection, could be causing the problem. I'll make sure to allocate some time to debug the APM and figure out what's going on with the contextvar.

Thanks for your time

@basepi
Copy link
Contributor

basepi commented Feb 14, 2024

Alright, you've confirmed my suspicion. Our middleware must be first in the list if you're passing in a list of middleware, but last if you're adding each one manually with add_middleware. I'll add a commit to your PR clarifying that point.

Edit: Ah, I can't push a commit because you used your main branch for this PR. I'll put it in a review instead.

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change. Thanks again for talking through this with me!

docs/starlette.asciidoc Outdated Show resolved Hide resolved
@mariocandela
Copy link
Contributor Author

One change. Thanks again for talking through this with me!

thank you for your time!

@mariocandela mariocandela requested a review from basepi February 16, 2024 07:26
@basepi
Copy link
Contributor

basepi commented Feb 21, 2024

run docs-build

@basepi basepi enabled auto-merge (squash) February 21, 2024 04:06
@basepi
Copy link
Contributor

basepi commented Feb 21, 2024

run docs-build

@basepi basepi merged commit 58c916b into elastic:main Feb 21, 2024
5 checks passed
xrmx pushed a commit that referenced this pull request Mar 20, 2024
* Update starlette.asciidoc

Fix typo

* Update starlette.asciidoc

---------

Co-authored-by: Colton Myers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-python community Issues opened by the community triage Issues awaiting triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants