Skip to content
This repository has been archived by the owner on Apr 27, 2021. It is now read-only.

disable default monitoring #161

Merged
merged 1 commit into from
Dec 3, 2018
Merged

Conversation

ElvinEfendi
Copy link

What this PR does / why we need it:
We don't use any stats from default monitoring. And we know that it's causing the controller to use a lot of resource: kubernetes#3314

This PR disables that.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

@Shopify/edgescale

@ElvinEfendi ElvinEfendi requested a review from wayt November 23, 2018 14:55
@ElvinEfendi
Copy link
Author

Image released index.docker.io/shopify/nginx-ingress-controller:0.21.0-no-monitor

@ElvinEfendi
Copy link
Author

I deployed this to tierstaging-central just to make sure it does not break anything and it seems to be working fine

@ElvinEfendi ElvinEfendi force-pushed the disable-default-monitoring branch from 3f01783 to 04c36a1 Compare November 27, 2018 11:52
@wayt
Copy link

wayt commented Nov 27, 2018

Can we close this now? Or should we make this an option on our ingress ?

@ElvinEfendi
Copy link
Author

@wayt I'm leaning toward merging this. Any objection? The current plan for upstream is to introduce a flag to disable this monitoring, but until that happens we can merge this PR IMO.

@ElvinEfendi
Copy link
Author

ElvinEfendi commented Nov 29, 2018

Another reason to merge this PR: https://logs.shopify.io/en-US/app/search/report?sid=_cGVlci5hbGxhbg_cGVlci5hbGxhbg__search__RMD5351807261e537e5c_at_1543437786_1468_6376F8BF-5E5C-4E4C-9BF8-1E079BB8768F&s=%2FservicesNS%2Fnobody%2Fsearch%2Fsaved%2Fsearches%2FDataDog%2520Top%2520Metrics%2520with%2520Cost

If you ctrl+f for ingress_nginx in that page you'll see we spends thousands $ per month.

That being said I'm surprised to see them there because we have disabled DD Prometheus checks.

@wayt
Copy link

wayt commented Nov 29, 2018

@ElvinEfendi 👍 :shipit: we're using statsd instead of this right?

@ElvinEfendi
Copy link
Author

we're using statsd instead of this right?

correct

@ElvinEfendi ElvinEfendi merged commit 44af211 into master Dec 3, 2018
@ElvinEfendi ElvinEfendi deleted the disable-default-monitoring branch December 3, 2018 19:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants