Skip to content
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

[Mellanox] Correct the wrong log identifiers of platform daemons #3596

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

stephenxs
Copy link
Collaborator

- What I did
Correct the wrong log identifiers of platform daemons on Mellanox platform
Originally the Logger class was introduced for the daemons running in pmon docker to provide them the ability to log message in an easy way with log identifier initialized as daemon's name.
When platform API is developed the Logger was also used by them with log identifiers initialized as the classes' name.
However, the logger identifier is global of the entire process scope, which means only one instance of log identifier for the entire daemon. As a result, the classes who is lastly loaded will have its classname set to the daemon's log identifier, causing the identifier incorrect in the logging message.
Since the original meaning of log identifier is the name of the daemon but not the modules, only daemons is legal to set the log identifier. All classes shouldn't set the log identifier when instantiate Logger class.

- How I did it
Don't touch log identifier when initialized by classes in platform API.

- How to verify it
config reload and check whether the pmon logs are correct.

- Description for the changelog

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

@jleveque
Copy link
Contributor

I think it might be even better if each class doesn't need to instantiate the Logger class. Maybe the new platform API itself could hold a global Logger class which is passed to it upon initialization. I just feel it isn't clear why in some instances the syslog identifier isn't passed into the constructor. This would also eliminate the need for each class to import sonic_daemon_base. I'm OK with merging this as a temporary solution, but I think we should consider a better long-term solution.

@stephenxs
Copy link
Collaborator Author

stephenxs commented Oct 13, 2019

I think it might be even better if each class doesn't need to instantiate the Logger class. Maybe the new platform API itself could hold a global Logger class which is passed to it upon initialization. I just feel it isn't clear why in some instances the syslog identifier isn't passed into the constructor. This would also eliminate the need for each class to import sonic_daemon_base. I'm OK with merging this as a temporary solution, but I think we should consider a better long-term solution.

Personally, I like your idea but I gave up doing so after reflecting on its impacts.
To do so requires some other modules to be updated as well:

  1. The loggers are instantiated in daemons. To pass loggers to platform API the daemons need to be updated, passing a "logger" argument when loading platform API.
  2. The prototype of the constructors of classes in sonic_platform_base, at least class PlatformBase, needs to be updated, adding a "logger" argument.
  3. The vendor implemented class Platform in sonic_platform also needs to be updated to align with PlatformBase.

In addition, all updates listed above have to be committed together otherwise the platform API will be broken and the daemons fall back to the plugin model.
So, currently, I don't prefer to change so much especially not long before the code frozen.
How do you think?

@jleveque
Copy link
Contributor

@stephenxs: I agree that my suggestion would be a pretty major change, so I agree with you to wait until after the 201910 release branch is created.

@jleveque
Copy link
Contributor

Retest this please

@jleveque jleveque changed the title [Mellanox]Correct the wrong log identifiers of platform daemons [Mellanox] Correct the wrong log identifiers of platform daemons Oct 15, 2019
@jleveque jleveque merged commit aea09ba into sonic-net:master Oct 15, 2019
@stephenxs stephenxs deleted the fix-pmon-logging-error branch October 15, 2019 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants