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

Adds WATCHMAN_DISABLE_APM option #115

Merged
merged 6 commits into from
Feb 7, 2018
Merged

Conversation

xfxf
Copy link
Contributor

@xfxf xfxf commented Jan 31, 2018

I've added a WATCHMAN_NEWRELIC_IGNORE option, which when enabled, disables instrumentation in the New Relic agent (used in many Django production installations) for any watchman calls.

This was done because I'm using watchman with ECS (on AWS) to monitor containers, and having watchman being hit multiple times every 30 seconds was causing a lot of New Relic stats that are based on averages to report totally inaccurately (throughput, apdex score, transaction times, etc) as the watchman queries end up being dominant.

There's no test, but I'm not clear how exactly I could test this outside of making the newrelic agent a dependency (which we don't want) or mocking the library completely (which makes me question what we're testing). The code is written defensively.

@coveralls
Copy link

coveralls commented Jan 31, 2018

Coverage Status

Coverage decreased (-1.7%) to 85.911% when pulling 50b02f0 on xfxf:master into 806e2e9 on mwarkentin:master.

@mwarkentin
Copy link
Owner

@xfxf Thanks for the suggestion! The first thing that comes to mind is whether it makes sense to call New Relic out specifically, or should we have a more general WATCHMAN_DISABLE_APM setting or similar?

New Relic is likely the most popular, but it could be nice to have a place to slot in support for removing from Datadog APM, AppDynamics, etc. without adding new settings for each.

What do you think?

@mwarkentin mwarkentin added this to the 0.15 milestone Jan 31, 2018
@xfxf
Copy link
Contributor Author

xfxf commented Feb 1, 2018

Agreed, but I don't use the other APM tools, so I wouldn't be able to easily test this.
I can refactor to make this a more generic name?

@mwarkentin
Copy link
Owner

@xfxf Yep, that's fine with me to just start with NR (under a more generic name).

@xfxf
Copy link
Contributor Author

xfxf commented Feb 1, 2018

@mwarkentin done, see commit above.

@mwarkentin
Copy link
Owner

Thanks! I’ll try to take a more in depth look later today!

@mwarkentin mwarkentin changed the title Adds WATCHMAN_NEWRELIC_IGNORE option Adds WATCHMAN_DISABLE_APM option Feb 5, 2018
@mwarkentin
Copy link
Owner

@xfxf can you resolve the conflicts with master?

Copy link
Owner

@mwarkentin mwarkentin left a comment

Choose a reason for hiding this comment

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

@xfxf Hey, sorry about the delay! Left a few small comments on the docs / code.

If you could add an item to the history file as well that'd be great!

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
watchman/views.py Outdated Show resolved Hide resolved
watchman/views.py Outdated Show resolved Hide resolved
watchman/views.py Outdated Show resolved Hide resolved
@xfxf
Copy link
Contributor Author

xfxf commented Feb 7, 2018

@mwarkentin done.

Copy link
Owner

@mwarkentin mwarkentin left a comment

Choose a reason for hiding this comment

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

Made a couple of small formatting tweaks to the docs, lgtm!

@mwarkentin mwarkentin merged commit 5be4abb into mwarkentin:master Feb 7, 2018
@mwarkentin
Copy link
Owner

Merged, thanks! I have one more update I want to include in the next release and then I'll push it up to pypi!

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.

3 participants