I’ve been thinking about this and we have a notification framework. Should we trigger that (as well or instead of) so admins get a notification? Maybe it’s too intrusive, but most admins don’t scan the logs that often and this pushes them more actively.
UI notifications would be good provided that they don’t spam notifications upon each API call and have very clear messaging on the call made and how to remedy. Sending the messages currently being logged as notifications would probably not be useful.
This should help me with finding out which script or tool was making the API call.
Thinking out loud now. We can log this to a file named deprecations-MAJOR.MINOR.log. If the file is created, a notification is created that the file can be read. It’s only appended to and not rotated. If a user thinks they’ve fixed all deprecations, they remove/rotate the file. When a new file is created, a new notification pops up again and the user knows to look into it. By including the version number, we ensure that when a user upgrades a new file is created.
I really like this idea. By increasing the visibility we can feel more secure in removing deprecated code. Right now it makes me a little apprehensive even for very old deprecations as I’m sure there are users who never would have seen them in the production log (regardless of $SUBJECT). Not to mention possible gaps in omitting deprecations from release notes or making the appropriate API doc amendments.
In the past few versions we have used UI notifications for deprecation notification, for example:
We have two blueprints already for deprecation notifications that can be used for features and settings at:
and it is easy to add more if needed (e.g. for API deprecations).
Instead of having to deal with files to track these and identify when a notification should be presented, why not generalize the code that was used to create the notifications before into something that is reusable and only creates a notification once if it doesn’t exist that can be trigger from anywhere? in this case, a deprecated api call will create a notification that will only be shown once. The notification can contain further information if we want it to - for example, which user used the call or what was the request id of the first call, or even send the notification to the user as well as the admins.
No, this is not a good idea. Users have nothing to do with code deprecations, they are simply not good target group. We are. And deprecations already have a good feature - they actually fail in test environment.
I am actually against this PR, this is designed well I think:
deprecation warnings do nothing in production mode
deprecation warnings do show up in development mode
deprecation warnings do raise exception in test mode (if they pass version threshold)
We have the best of all three worlds. It’s important to realize that we will always have some deprecation warnings - that’s why they do exits: to warn developers and to span for some time so plugin authors can adopt.
I think it’s not black or white only. There are deprecations which are important for admins (not regular users). Admins should know their scripts or users are using e.g. deprecated API endpoints and should migrate the tooling to use new ones. That’s what I believe @ekohl suggests.
Then there are deprecations of internal API (aka deprecated rubyge methods etc) which admins can’t really do much about. This is what I believe @lzap has in mind.
Therefore I think we need to distinguish in between the two types of deprecations - code deprecations and API deprecations. Does that make sense?
Yeah I actually changed my mind in the PR comment as I digged deeper into the problem. I think a warning message would be fine, but I’d like to keep using ActiveRecords deprecations, thus only adding the warning message on top of that. In Rails 6 it will be possible to specify warnings which are treated differently - this could be useful.
I agree, I see no reason for closing the PR. I was quite pessimistic with the change in the first place, but this was because it comes without any description and I had no context. I think it’s quite reasonable to remove ActiveSupport::Deprecation.behavior = :silence if Rails.env.production? from our codebase but since I expect that customers will complain about millions of warning messages in logs, we should create a setting that will silence those if needed.
So it could be a two-liner patch. Change of the line mentioned and new setting in settings.yaml.example explaining the new value. Preferably a new installer/puppet option but here is the issue I’ve pointed on already - putting every single detail into installer makes it overhelming. I believe we should have two tiers of settings - one managed by the installer and one for very detalied things like this one. This is another discussion I guess.
I don’t know how we silence those without also silencing our own deprecation warnings currently. And I think most users will just turn them off if the ones they care about are a small % compared to ones they don’t care about.
Isn’t the goal to actually show them? What I suggest is:
if Rails.env.production? && Settings[:silence_deprecation_warnings]
ActiveSupport::Deprecation.behavior = :silence
Another approach is to create a new named logger and make sure all deprecations get logged through it. It can be turned on by default but users would be able to turn it off just like any other logger like sql or ldap.
Finally an explanation of the problem that makes sense to me.
Yeah, well it’s possible in Rails 6 but that’s not on table at the moment.
I am good with doing the original proposal but into a separate logger. Let’s use a separate name like “deprecated_api” so we can add “deprecated_rails” or “deprecated_app” later on as it might be useful to show these too.