Use rails deprecation behavior but log in production #18847

jrafanie · 2019-06-10T15:47:06Z

Copy current edge rails defaults for test/dev
Log deprecations in production mode
Change from rails :notify default since it's unlikely we'll have users setup notifications for deprecation warnings.

There is no technical difference but clean up the settings.

Change from rails :notify default since it's unlikely we'll have users setup notifications for depecation warnings.

jrafanie · 2019-06-10T15:48:35Z

config/environments/production.rb

+
+  # Change from rails :notify default since it's unlikely we'll have users setup
+  # notifications for deprecation warnings.
+  config.active_support.deprecation = :log


This is the 🍖 of the PR. The default of notify is great when hosted as you can setup notifications for these. Since most of our users are not hosted by us, it will be unlikely we'll ever get deprecations reported to us unless we get them by default from logging.

I'm not sure...see my comment below. What does this solve that we couldn't do previously?

Deprecations, especially rails ones, should be exceptional. I'd like to stay on top of them so future upgrades are easier. Note, this doesn't affect Vmdb::Deprecation as that will never log in production so this only logs rails deprecations, ones we should definitely stay on top of.

Don't we have a lot of code suppressing the notifications in production? At least for our own deprecations.

UPDATE: ugh, you just said that

jrafanie · 2019-06-10T15:48:51Z

config/environments/test.rb

-  # Print deprecation notices to the stderr
-  #ActiveSupport::Deprecation.behavior = :stderr
+  # Print deprecation notices to the stderr.
+  config.active_support.deprecation = :stderr


Fix the syntax for this to the defaults from rails.

bdunne

👍 LGTM

Fryguy · 2019-06-10T16:17:59Z

~~Does this work with the container logger? I ask because of the stderr bit.~~ nvm...didn't catch that the stderr bit was for test only

Fryguy · 2019-06-10T16:19:09Z

We've classically had deprecations off because in production mode (i.e. at customers) it's just noise. It's not like they can fix it.

Fryguy · 2019-06-10T16:22:20Z

config/environments/production.rb

-  # Send deprecation notices to registered listeners
-  config.active_support.deprecation = :notify
+  # Send deprecation notices to registered listeners.
+  # config.active_support.deprecation = :notify


Does a vanilla production.rb have both lines, but one commented out? If not, then I would think we'd just delete this line.

Right. I left it because the comment is wrong with this line saying :log instead of :notify

If I drop the the line that ships with rails, I should drop the comment too. They should stay or go together.

bdunne · 2019-06-10T17:29:12Z

We've classically had deprecations off because in production mode (i.e. at customers) it's just noise. It's not like they can fix it.

We shouldn't go out the door with any known Rails deprecation warnings, right? We need to find out about any that remain and fix them before those methods are removed or changed. (and add tests)

miq-bot · 2019-06-10T17:46:02Z

Checked commits jrafanie/manageiq@bac06ac~...201ebfe with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

jrafanie · 2019-06-10T17:53:51Z

We've classically had deprecations off because in production mode (i.e. at customers) it's just noise. It's not like they can fix it.

We shouldn't go out the door with any known Rails deprecation warnings, right? We need to find out about any that remain and fix them before those methods are removed or changed. (and add tests)

👍 The reason to do this is to stay on top of fixing deprecations so they're not noisy. Any logs we receive we can look for deprecations and try to stay in front of them so upgrades are easier.

kbrock

Printing out deprecations is great.

Not sure the best way to encourage people to actually fix these deprecations rather than just live with them.

kbrock · 2019-06-10T18:18:23Z

config/environments/production.rb

+
+  # Change from rails :notify default since it's unlikely we'll have users setup
+  # notifications for deprecation warnings.
+  config.active_support.deprecation = :log


Don't we have a lot of code suppressing the notifications in production? At least for our own deprecations.

UPDATE: ugh, you just said that

NickLaMuro

My 2Cents (aside from the snark): I think this is a fine approach. Effectively we are crowd source our deprecation hunting, and I think that is a reasonable approach since we won't usually be able catch this ourselves.

Something to think about as well: Possibly we put together a production log parsing script that we can take user submitted logs and spend some time looking for deprecation warnings as well that crop up. Maybe filters on uniqueness and uses counts?

NickLaMuro · 2019-06-10T20:48:52Z

config/environments/development.rb

@@ -19,7 +19,7 @@
  # Don't care if the mailer can't send
  config.action_mailer.raise_delivery_errors = false

-  # Print deprecation notices to the Rails logger
+  # Print deprecation notices to the Rails logger.


Man, this line is SUPER important!

Man, this line is SUPER important!

Period.

NickLaMuro · 2019-06-10T20:50:48Z

config/environments/test.rb

@@ -10,8 +10,8 @@
  config.cache_classes = true
  config.eager_load = false

-  # Print deprecation notices to the stderr
-  #ActiveSupport::Deprecation.behavior = :stderr
+  # Print deprecation notices to the stderr.


Again, extremely important!

jrafanie · 2019-06-10T21:01:49Z

Something to think about as well: Possibly we put together a production log parsing script that we can take user submitted logs and spend some time looking for deprecation warnings as well that crop up. Maybe filters on uniqueness and uses counts?

@NickLaMuro I used this for the logs generated from QE:
zgrep -E 'DEPRECATION' */log/*.log* | grep -oE "WARN.+" | sort | uniq -c

It's not perfect but grouped enough of them together to get a decent idea of how many times it happens.

NickLaMuro · 2019-06-10T21:15:38Z

@NickLaMuro I used this for the logs generated from QE:
$ zgrep -E 'DEPRECATION' */log/*.log* | grep -oE "WARN.+" | sort | uniq -c
It's not perfect but grouped enough of them together to get a decent idea of how many times it happens.

That works, though this got me thinking: Does this make sense as something that runs as an after task in CI as well, and then we can hook it into the @miq-bot (via a webhook) or something that could then bug people on PRs directly?

Again, just tossing some ideas out there for consideration. Not in anyway something that needs to be actionable right now.

jrafanie added 2 commits June 10, 2019 10:59

Copy current edge rails defaults for test/dev

bac06ac

There is no technical difference but clean up the settings.

Log deprecations in production mode

201ebfe

Change from rails :notify default since it's unlikely we'll have users setup notifications for depecation warnings.

jrafanie commented Jun 10, 2019

View reviewed changes

bdunne approved these changes Jun 10, 2019

View reviewed changes

Fryguy reviewed Jun 10, 2019

View reviewed changes

kbrock approved these changes Jun 10, 2019

View reviewed changes

NickLaMuro approved these changes Jun 10, 2019

View reviewed changes

bdunne merged commit 3899308 into ManageIQ:master Jun 10, 2019

bdunne self-assigned this Jun 10, 2019

bdunne added this to the Sprint 113 Ending Jun 10, 2019 milestone Jun 10, 2019

jrafanie deleted the use_rails_deprecation_behavior_but_log_in_production branch June 10, 2019 21:23

bdunne added changelog/yes core rails5.1 labels Jun 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rails deprecation behavior but log in production #18847

Use rails deprecation behavior but log in production #18847

jrafanie commented Jun 10, 2019

jrafanie Jun 10, 2019

Fryguy Jun 10, 2019

jrafanie Jun 10, 2019

kbrock Jun 10, 2019 •

edited

Loading

jrafanie Jun 10, 2019

bdunne left a comment

Fryguy commented Jun 10, 2019 •

edited

Loading

Fryguy commented Jun 10, 2019 •

edited

Loading

Fryguy Jun 10, 2019

jrafanie Jun 10, 2019

jrafanie Jun 10, 2019

bdunne commented Jun 10, 2019 •

edited

Loading

miq-bot commented Jun 10, 2019

jrafanie commented Jun 10, 2019

kbrock left a comment

kbrock Jun 10, 2019 •

edited

Loading

NickLaMuro left a comment

NickLaMuro Jun 10, 2019

jrafanie Jun 10, 2019

NickLaMuro Jun 10, 2019

jrafanie commented Jun 10, 2019

NickLaMuro commented Jun 10, 2019

Use rails deprecation behavior but log in production #18847

Use rails deprecation behavior but log in production #18847

Conversation

jrafanie commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne left a comment

Choose a reason for hiding this comment

Fryguy commented Jun 10, 2019 • edited Loading

Fryguy commented Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne commented Jun 10, 2019 • edited Loading

miq-bot commented Jun 10, 2019

jrafanie commented Jun 10, 2019

kbrock left a comment

Choose a reason for hiding this comment

kbrock Jun 10, 2019 • edited Loading

Choose a reason for hiding this comment

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Jun 10, 2019

NickLaMuro commented Jun 10, 2019

kbrock Jun 10, 2019 •

edited

Loading

Fryguy commented Jun 10, 2019 •

edited

Loading

Fryguy commented Jun 10, 2019 •

edited

Loading

bdunne commented Jun 10, 2019 •

edited

Loading

kbrock Jun 10, 2019 •

edited

Loading