Skip to content

Conversation

mitsuhiko
Copy link
Contributor

No description provided.

@untitaker
Copy link
Member

Since urllib3 uses httplib I wonder if this would double-log requests when using urllib3 or requests (or an older version of requests that uses stdlib urllib)

@@ -9,11 +9,13 @@

def _get_default_integrations():
from .logging import LoggingIntegration
from .stdlib import StdlibIntegration
Copy link
Member

Choose a reason for hiding this comment

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

probably meant to be httplib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make an stdlib integration with all the hooks against the stdlib instead of one integration per stdlib module

Copy link
Member

Choose a reason for hiding this comment

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

ok, should we move the logging integration into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would keep that one separate because it's so special.

Copy link
Member

Choose a reason for hiding this comment

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

Keep in mind though that if we put too much into one integration you can't enable/disable each of the "sub-integrations" one-by-one.

I don't understand why you do this though since the stdlibintegration is enabled by default anyway.

@mitsuhiko
Copy link
Contributor Author

I don't think it dual logs as urllib3 does not use httplib i think. At least we did the same thing in the old lib and did not dual record. Will test though

@mitsuhiko mitsuhiko force-pushed the feature/http-integrations branch from b565509 to 862ee26 Compare September 6, 2018 12:47
@mitsuhiko mitsuhiko force-pushed the feature/http-integrations branch from 862ee26 to 24a3cec Compare September 6, 2018 13:08
@untitaker
Copy link
Member

can't repro this bs locally so let's print out local vars

@untitaker untitaker force-pushed the feature/http-integrations branch 2 times, most recently from 35f678c to 8ae46fc Compare September 7, 2018 08:12
@untitaker untitaker force-pushed the feature/http-integrations branch from 8ae46fc to b8a9743 Compare September 7, 2018 09:04
@untitaker untitaker self-assigned this Sep 7, 2018
@untitaker
Copy link
Member

@mitsuhiko please check if this works on your machine. I magically managed to reproduce the travis failure this morning

@untitaker untitaker merged commit d1bf63f into master Sep 11, 2018
@untitaker untitaker deleted the feature/http-integrations branch September 11, 2018 12:40
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