[sonic_daemon_base]Fix error: DaemonBase: object has no attribute 'syslog' #4034

[stephenxs]

- What I did
Fix error: DaemonBase: object has no attribute 'syslog' #4034

- How I did it
syslog is not a member of class DaemonBase, so remove the "self." in DaemonBase.
See issuecomment-575678314 for detail.

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[jleveque]

This should fix the issue, but I wonder if we should utilize the Logger class here, since it is defined in this file. In order to do so, we would need to pass the syslog_identifier to the DaemonBase constructor, however, so that it can instantiate the Logger class. How do you feel about taking this type of approach?

[stephenxs]

Your approach is better except that it could introduce a bit more effort because current all derived classes of DaemonBase don't use the logger as a member of class but as a global variable in the file. We need to update the derived classes as well.
If we go this way, we can update the base class first and then update the derived classes in the future.
Meanwhile, the approach in this PR can also achieve the purpose of initializing the log_identifier but at a different level. What I mean is that the python modules importing this module will have their own Logger instance and initialize it as well. By doing so the log_indentifier information initialized in daemon will be overwritten. This is why we have the (PR 3596)[https://github.com//pull/3596/files].
How do you think?

[jleveque]

If we go this way, we can update the base class first and then update the derived classes in the future.

I agree.

Meanwhile, the approach in this PR can also achieve the purpose of initializing the log_identifier but at a different level. What I mean is that the python modules importing this module will have their own Logger instance and initialize it as well. By doing so the log_indentifier information initialized in daemon will be overwritten. This is why we have the (PR 3596)[https://github.com//pull/3596/files].
How do you think?

I don't quite understand what kind of impact making the change I suggested would have on the issue that was worked around by #3596. Could you please elaborate?

[stephenxs]

Sorry, I don't express it clearly. I don't mean it will have impact.
I meant another possible approach in which Logger class is initiated as a global variable in the daemon_base.py rather than a class member in DaemonBase. By doing so we don't need to pass the log_identifier from derived classes to DaemonBase but just need to initiate Logger instance as a global in the files that importing daemon_base.py.
Is it clear?

[jleveque]

I see your point now. I guess I feel that since the Logger class is defined in the daemon_base.py file, it is intended to be either inherited by DaemonBase or used as a member of the class. If we want to use them separately (e.g., a class which derives from DaemonBase will create a global Logger instance), I think we should consider breaking Logger out into its own file in the future. We should consider creating a "sonic_tools" (or similar) Python package, which can house daemon_base.py, logger.py, minigraph.py, port_config.py, and all other Python libraries which are shared by mulitple SONiC Python programs. This, of course, is a bit of a large project, so I think for this fix, we should choose a solution which will work best with that future direction in mind.

Since Python is object-oriented, I'd prefer to avoid global variables, if possible. What is there to be gained by adding a global Logger instance in DaemonBase versus adding a member variable? I think it makes subclassing DaemonBase easier to simply pass the log_identifier when construction the class, then the inheriting class automatically has a working Logger, rather than having to instantiate a separate global Logger variable. However, if a program which isn't a daemon just wants a logger, it can import the logger.py file and only take that class.

As an alternative to the fix in #3596, maybe the platform API could take callback functions for logging instead of having global variables. That way, the platform API would call the callback functions of the running daemon rather than maintaining Logger state themselves. What do you think?

[stephenxs]

Personally, I tend to agree with you except that we need to come up with a good way regarding how Logger is used in platform API.

[jleveque]

Agreed. Let's go ahead with this fix in the interim, and then discuss a good way to use Logger in the platform API. You can take this PR out of "Draft" state.

[jleveque]

Retest vs please

[jleveque]

Retest vsimage please

[jleveque]

Retest vs please

[jleveque]

Retest vsimage please

[stephenxs]

retest vsimage please

[lguohan]

retest vs please

[jleveque]

retest vsimage please

[stephenxs]

retest vsimage please

[jleveque]

Retest vsimage please

[stephenxs]

retest vsimage please