-
Notifications
You must be signed in to change notification settings - Fork 428
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
Deprecate mam02 #1514
Deprecate mam02 #1514
Conversation
ejabberd.log from Build #3625/Job #3625.3 - entry with the error: EDIT: After a small commit (#6472465) the mesage should look like this: |
@fenek, is it waiting to be merged or needs some corrections (and you did not change the label nor make any comment on that)? |
%% Logs as ERROR message. | ||
-spec log(deprecation() | string()) -> ok. | ||
log(mam02_archived) -> | ||
maybe_log_mamv02_deprecation_error(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not very generic. This module shouldn't provide an implementation specific for certain deprecations, as it unnecessarily increases coupling. The API should be log(Tag :: atom(), Level :: atom(), FormatStr :: string(), Params :: list())
(with log(Tag, FormatStr, Params)
may still default to error
log level). There shouldn't be per-deprecation table but one global for all deprecations and its elements' keys should be {Tag, last_logged}
or just Tag
. I think we may stick to common cooldown time for all deprecations for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of this module as tightly coupled with those that want to "call themselves" deprecated, as a module that knows about specific modules and the way each of them wants to annnounce the deprecation. So that now module itself just needs to trigger log/1
function whenever it wants to log deprecation message and here is the logic responsible for cooldowns, log format, preparing necessary ETS tables etc.
What would be the gain for creating a module with only a log message?
Moreover - it wouldn't be too generic if we say each deprecation has some kind of cooldown. Maybe some deprecation messages are shown at Mongoose's start or shutdown and not triggered by some event as <archived/>
is?
I see this module as one that:
- exposes API for starting and stoping it (where it creates all the stuff that is needed to handle different types of deprecations e.g. creating ETS tables or registering hooks
- exposes API to log such a warning
@fenek, tell me how is your vision different from this one so that I can make necessary corrections. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make it the other way round:
make mongoose_deprecations' functions callbacks and make modules implement them. That would loosen the coupling between them and this new module but on the other hand we will have this quite a huge piece of code that is only responsible for deprecating (which is not part of the main job of the module).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also go in the direction that all deprecations look alike - they have some cooldown and if module calls our log
function we first check if it's not logged too early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, as I guess you probably meant the third options, I will stick to that
Do we want the list of deprecation tags to be loaded from config (possibly with their cooldown times), @fenek? Or do you want it the way it is now (or any other suggestions?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer your question: Until there is a real need to fine-tune the warning frequency, I don't think it should be configurable at all. 6h is a good interval to be annoying but not a performance issue.
|
||
-include("ejabberd.hrl"). | ||
|
||
-type deprecation() :: atom(). | ||
-define(DEPRECATIONS, [mam02]). % List of deprecation tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module shouldn't be aware of how many deprecations there are.
ets:new(?DEPRECATION_TAB, [{read_concurrency, true}, named_table, public]), | ||
ets:insert(?DEPRECATION_TAB, {last_logged, not_logged}), | ||
lists:foreach(fun(Tag) -> | ||
ets:insert(?DEPRECATION_TAB, {Tag, not_logged}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to pre-fill this table.
30e87f3
to
4763957
Compare
@fenek, I have implemented tests. You can take a look ;) |
@fenek, I have doubts about this test. Firstly, to be able to mock the function from the module I test I had to call it this way: I'm mocking function that directly calls lager:warning/2 or lager:error/2. I found it very hard to mock lager as its functions are created in parse transform so they don't exist when I want to mock them. |
367b2e3
to
cebd6ad
Compare
We decided that all deprecation messages will need the same system - cooldowns. So this commit makes it common.
d35a6c1
to
ba5ae84
Compare
This PR adds a new module responsible for maintaining deprecations messages