miketheman / heroku-buildpack-datadog

Heroku Buildpack to run Datadog DogStatsD in a Dyno
http://miketheman.github.io/heroku-buildpack-datadog
MIT License
55 stars 35 forks source link

Disable DataDog agent in the proper way #24

Closed bronislav closed 7 years ago

bronislav commented 7 years ago

After #8 there is a way to selectively disable agent.

That approach does not work as expected. Dynos are crashed after this https://github.com/miketheman/heroku-buildpack-datadog/blob/master/extra/run-dogstatsd.sh#L5 line. This is happening because of the "Dyno crash restart policy":

A dyno “crash” represents any event originating with the process running in the dyno that causes the dyno to stop. That includes the process exiting with an exit code of 0 (or any other exit code).

This PR looks like a proper fix

miketheman commented 7 years ago

Hi @bronislav !

Thanks for pointing this out - it must have slipped through.

In surfacing this, I'm wondering if we're doing it the wrong way all along.

The Heroku Buildpack API has the detect script that, if exit 0, uses this buildback. https://devcenter.heroku.com/articles/buildpack-api#bin-detect

I wonder if it is more appropriate to move the envvar detection to the detect script, so as to prevent the entire package installation to begin with.

I'm not sure which is a better method yet, so let's discuss. /cc @dreid for previous approach

bronislav commented 7 years ago

@miketheman I thought about a way to manage agent by env variables.

About "detect" approach. Detect script runs during "build" phase and there is no way to execute it without rebuilding slug (obviously). But it is OK to rebuild slug if we can make it working.

miketheman commented 7 years ago

@bronislav I'm not sure I understand the part about needing to rebuild the slug.

I thought that as each buildpack starts, it looks for the conditions in detect and then acts on those. The action of setting/removing an envvar triggers a redeploy, does it not?

bronislav commented 7 years ago

@miketheman setting/removing env var triggers dyno restart with new environment, but not slug (application container) rebuild. Rebuild is triggered only on the deployment of new code.

miketheman commented 7 years ago

Thanks!