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

Alternative to #23638 log to STDOUT via env var #23734

Merged
merged 1 commit into from Feb 23, 2016

Conversation

Projects
None yet
3 participants
@schneems
Member

schneems commented Feb 17, 2016

People who deploy to containers or other places where they might have some sort of a log wrapping service use stdout. With this change new rails apps can be configured to output to STDOUT via setting RAILS_LOG_TO_STDOUT to any value. This allows container images or services to set the value for all apps without having to modify configuration for each application. If an app wants to opt out, they can either delete from the env hash, or remove that configuration.

cc/ @rafaelfranca

Alternative to #23638 log to STDOUT via env var
People who deploy to containers or other places where they might have some sort of a log wrapping service use stdout. With this change new rails apps can be configured to output to STDOUT via setting `RAILS_LOG_TO_STDOUT` to any value. This allows container images or services to set the value for all apps without having to modify configuration for each application. If an app wants to opt out, they can either delete from the env hash, or remove that configuration. 

cc/ @rafaelfranca

@maclover7 maclover7 added the railties label Feb 17, 2016

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 17, 2016

Member

huge 👎 to adding new env vars. We already have a lot and I really don't like the idea of environment variables. We have environment configurations files so lets use that. If people want to log to STDOUT they can just change the production.rb file.

Member

rafaelfranca commented Feb 17, 2016

huge 👎 to adding new env vars. We already have a lot and I really don't like the idea of environment variables. We have environment configurations files so lets use that. If people want to log to STDOUT they can just change the production.rb file.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 17, 2016

Member

Let me try again since I'm ditching your work without trying to understand. Why environment variable is better than have people changing their environment file? I know there are contained environments but when you make a choice to deploy to these environments could not you also change your application so it works in the environment?

Member

rafaelfranca commented Feb 17, 2016

Let me try again since I'm ditching your work without trying to understand. Why environment variable is better than have people changing their environment file? I know there are contained environments but when you make a choice to deploy to these environments could not you also change your application so it works in the environment?

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Feb 17, 2016

Member

It's only 1 more env var and it lets me kill https://rubygems.org/gems/rails_12factor 🙏. I promise no more env var pull requests for at least a year 😄

when you make a choice to deploy to these environments could not you also change your application so it works in the environment?

I want that "just works" (TM) experience with my apps. Most people don't care about the minutia of running on production, they want things to work out of the box. That is why we log to disk by default in production, instead of require a config change to do it. If I am an agency that is making new rails apps for different marketing sites every day then it is much easier for them to configure their environment once than to configure each new app every time. There's very few things that a container cares about to be able to run an app. The app has to know what port is available, the container needs to be able to capture logs, and in the case of Rails we want to be able to serve static files without needing an intermediate server such as nginx. These are the three configuration via environment variables experience that I am most interested in, and logging to STDOUT is the only one that doesn't work on master right now.

Member

schneems commented Feb 17, 2016

It's only 1 more env var and it lets me kill https://rubygems.org/gems/rails_12factor 🙏. I promise no more env var pull requests for at least a year 😄

when you make a choice to deploy to these environments could not you also change your application so it works in the environment?

I want that "just works" (TM) experience with my apps. Most people don't care about the minutia of running on production, they want things to work out of the box. That is why we log to disk by default in production, instead of require a config change to do it. If I am an agency that is making new rails apps for different marketing sites every day then it is much easier for them to configure their environment once than to configure each new app every time. There's very few things that a container cares about to be able to run an app. The app has to know what port is available, the container needs to be able to capture logs, and in the case of Rails we want to be able to serve static files without needing an intermediate server such as nginx. These are the three configuration via environment variables experience that I am most interested in, and logging to STDOUT is the only one that doesn't work on master right now.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 17, 2016

Member

Can't that be done using a generator template? rails new -t https://heroku.com/rails_template my_app.

That application would work out of box and would also let heroku or any contained environment go crazy and set what they thing it is important for them.

I want that "just works" (TM) experience with my apps.

I know. I want that too and the current configuration file "just works".

If I am an agency that is making new rails apps for different marketing sites every day then it is much easier for them to configure their environment once than to configure each new app every time.

You are describing app generator template feature.

These are the three configuration via environment variables experience that I am most interested in, and logging to STDOUT is the only one that doesn't work on master right now.

In fact I was against the RAILS_SERVE_STATIC_FILES environment variable too. And if I recall correctly that was the "last environment variable that we need". After it we added RAILS_MAX_THREADS and now this.

If all we want is to new application "just works" in contained environments I'd go with a generator template.

Member

rafaelfranca commented Feb 17, 2016

Can't that be done using a generator template? rails new -t https://heroku.com/rails_template my_app.

That application would work out of box and would also let heroku or any contained environment go crazy and set what they thing it is important for them.

I want that "just works" (TM) experience with my apps.

I know. I want that too and the current configuration file "just works".

If I am an agency that is making new rails apps for different marketing sites every day then it is much easier for them to configure their environment once than to configure each new app every time.

You are describing app generator template feature.

These are the three configuration via environment variables experience that I am most interested in, and logging to STDOUT is the only one that doesn't work on master right now.

In fact I was against the RAILS_SERVE_STATIC_FILES environment variable too. And if I recall correctly that was the "last environment variable that we need". After it we added RAILS_MAX_THREADS and now this.

If all we want is to new application "just works" in contained environments I'd go with a generator template.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Feb 17, 2016

Member

I'll trade you RAILS_MAX_THREADS for this feature. My hope has always been to find some sane way to get rid of the rails_12factor gem. It only does two things log to STDOUT and flip a switch to enable static file service. Having to discover that rails_12factor gem or a template based solution is similar to discovering the corner of a piece of furniture in a dark room with your toe. The knowledge comes at a fairly painful cost.

Here's the original un-merged PR for adding static file service #17347 then eventually added by @chancancode in #18100.

To my knowledge the presence of the static file PR hasn't had any significant cost to Rails maintenance. People either use it or ignore it. I could make this PR a somewhat dense one liner, if we're concerned with line count. If your primary concern is my slowly replacing EVERY command with environment variables, I promise to not do that. I've promoted using env vars previously but don't think I've ever made a "this is the last one" statement before.

Terence previously tried to get STDOUT logging service and it was reverted 1a90550#commitcomment-2819166 though the reasons given are related to the presence or not of logging files. I don't think that is a good approach as it is too heavy handed. My other closed PR was an attempt at a "why not both" type solution which also appeared heavy handed. This PR is a pretty minimal, non-magical configuration addition. I can't imagine this causing any kind of maintenance burden.

I hear what you're saying about templates. It still feels like the wrong solution. It seems like a very high overhead for configuring one value. I don't see it as any better than the rails_12factor gem. It might be worse for our case since there is a standard way to see if there is a gem in your Gemfile.lock but not to introspect Rails config in a machine readable way, which is perhaps another discussion.

Member

schneems commented Feb 17, 2016

I'll trade you RAILS_MAX_THREADS for this feature. My hope has always been to find some sane way to get rid of the rails_12factor gem. It only does two things log to STDOUT and flip a switch to enable static file service. Having to discover that rails_12factor gem or a template based solution is similar to discovering the corner of a piece of furniture in a dark room with your toe. The knowledge comes at a fairly painful cost.

Here's the original un-merged PR for adding static file service #17347 then eventually added by @chancancode in #18100.

To my knowledge the presence of the static file PR hasn't had any significant cost to Rails maintenance. People either use it or ignore it. I could make this PR a somewhat dense one liner, if we're concerned with line count. If your primary concern is my slowly replacing EVERY command with environment variables, I promise to not do that. I've promoted using env vars previously but don't think I've ever made a "this is the last one" statement before.

Terence previously tried to get STDOUT logging service and it was reverted 1a90550#commitcomment-2819166 though the reasons given are related to the presence or not of logging files. I don't think that is a good approach as it is too heavy handed. My other closed PR was an attempt at a "why not both" type solution which also appeared heavy handed. This PR is a pretty minimal, non-magical configuration addition. I can't imagine this causing any kind of maintenance burden.

I hear what you're saying about templates. It still feels like the wrong solution. It seems like a very high overhead for configuring one value. I don't see it as any better than the rails_12factor gem. It might be worse for our case since there is a standard way to see if there is a gem in your Gemfile.lock but not to introspect Rails config in a machine readable way, which is perhaps another discussion.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 17, 2016

Member

I'll sleep on it. I see your points too and they make sense.

Member

rafaelfranca commented Feb 17, 2016

I'll sleep on it. I see your points too and they make sense.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Feb 17, 2016

Member

Thanks for the extra consideration.

Member

schneems commented Feb 17, 2016

Thanks for the extra consideration.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Feb 23, 2016

Member

We talked about this in campfire. The change was deemed acceptable, merging in.

Member

schneems commented Feb 23, 2016

We talked about this in campfire. The change was deemed acceptable, merging in.

schneems added a commit that referenced this pull request Feb 23, 2016

@schneems schneems merged commit e9b96f0 into rails:master Feb 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

schneems added a commit to heroku/heroku-buildpack-ruby that referenced this pull request Feb 23, 2016

Enable STDOUT logging via rails/rails#23734
Do not warn when rails_12factor not enabled.

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Feb 24, 2016

rafaelfranca added a commit that referenced this pull request Feb 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment