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

Mesos: Prevent requests from using chardet #2192

Merged
merged 1 commit into from Feb 5, 2016
Merged

Mesos: Prevent requests from using chardet #2192

merged 1 commit into from Feb 5, 2016

Conversation

GregBowyer
Copy link
Contributor

Requests internally (and irritatingly imho) bundles chardet. It uses this library to figure out the encoding for a given response if there are no suitable HTTP headers detailing the character set of the response.

This library is pathologically slow (its loop heavy) on any python implementation not running on top of a VM. There are better implementations of chardet for vanilla pythons but the requests library authors wish to avoid bundling C extensions. https://github.com/kennethreitz/requests/issues/2359

Taking a leaf from opentable, we now force the encoding of the mesos response to always be UTF8 which avoids requests pulling large mesos payloads through chardet and prevents the datadog agent taking large amounts of CPU time to run the check.

Requests internally (and irritatingly imho) bundles `chardet`. It uses this library to figure out the encoding for a given response if there are no suitable HTTP headers detailing the character set of the response.

This library is pathologically slow (its loop heavy) on any python implementation _not_ running on top of a VM. There are better implementations of `chardet` for vanilla pythons but the requests library authors wish to avoid bundling C extensions. https://github.com/kennethreitz/requests/issues/2359

Taking a leaf from opentable, we now force the encoding of the mesos response to always be UTF8 which avoids requests pulling large mesos payloads through `chardet` and prevents the datadog agent taking large amounts of CPU time to run the check.
@talwai
Copy link
Contributor

talwai commented Jan 8, 2016

Hi @GregBowyer , thanks for contributing! Looks like a worthwhile change, would you in fact be able to provide some benchmarks on performance improvement as a result of your changes to the check? Agent Developer Mode might be helpful here.

@GregBowyer
Copy link
Contributor Author

Hi @GregBowyer , thanks for contributing! Looks like a worthwhile change, would you in fact be able to provide some benchmarks on performance improvement as a result of your changes to the check? Agent Developer Mode might be helpful here.

I could but I would prefer not to, this gave us serious performance issues in our datacenter. Essentially datadog no-data alarms for all our monitors would start firing with the mesos check enabled, which comes through pagerduty which wakes our support staff up :P.

@talwai
Copy link
Contributor

talwai commented Jan 8, 2016

No worries, and sincere apologies for any rude wake up calls this caused.
We'll profile the improvements on our end, and consider this for an
upcoming release

On Fri, Jan 8, 2016 at 4:22 PM Greg Bowyer notifications@github.com wrote:

Hi @GregBowyer https://github.com/GregBowyer , thanks for contributing!
Looks like a worthwhile change, would you in fact be able to provide some
benchmarks on performance improvement as a result of your changes to the
check? Agent Developer Mode might be helpful here.

I could but I would prefer not to, this gave us serious performance issues
in our datacenter. Essentially datadog no-data alarms for all our monitors
would start firing with the mesos check enabled, which comes through
pagerduty which wakes our support staff up :P.


Reply to this email directly or view it on GitHub
#2192 (comment).

@remh remh added this to the 5.7.0 milestone Jan 15, 2016
talwai added a commit that referenced this pull request Feb 5, 2016
Mesos: Prevent requests from using chardet
@talwai talwai merged commit 8311b7a into DataDog:master Feb 5, 2016
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.

None yet

3 participants