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

Capture full URL in urllib3 spans #587

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

adamchainz
Copy link
Contributor

Spotted whilst debugging a customer's traces.

@adamchainz
Copy link
Contributor Author

I think there's still a bug here where we sometimes capture the wrong hostname, due to a connection pool getting reused for different hosts. It seems requests does this as an optimization.

adamchainz and others added 2 commits April 9, 2021 11:44
Spotted whilst debugging a customer's traces.
While requests isn't instrumented directly, we do instrument it via urllib3.
These tests do a basic verification of that.
@tim-schilling
Copy link
Collaborator

I've dug into this and tried recreating it via requests and urllib3. I exceeded the pool size limits and increased the count to see if it was an odd bug exposed by repetition. I was unable to reproduce the bug. Looking at the code, I'm not sure how it would exist since we generate the absolute url from the wrapped connection instance.

If CI passes, I'm inclined to merge this.

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.

2 participants