Gracefully shutdown analytics module/runner

[zhongshixi]

Problem

1. I have an implementation of an adapter that requires explicit flushing of the data when the prebid server shutdown to ensure minimum data loss and I would like not to duplicate the signal notification logic written in the server listener.

Solution

1. I think the best solution is to provide a common interface Shutdown() so different analytic module could implement their own way of shutting down their module, if a module does not require a graceful shutdown, then just provide no-op implementation

[hhhjort]

Just a friendly ping, are you still working on this?

[zhongshixi]

@hhhjort yes, sorry for the late rely, i am slowing working on this, you can expect next week i will update the commit

[zhongshixi]

going to resume the work today

[AlexBVolcy]

For future reference, can you not force push as it squashes the commits from the past making it harder to review. Thank you!

[zhongshixi]

For future reference, can you not force push as it squashes the commits from the past making it harder to review. Thank you!

@AlexBVolcy thanks for pointing it out, the reason i force-push it is because the PR is based upon a very old version so i did git rebase - my intention is to have my very-old commit on the top of the latest commit head.

I will try to use merge back from master into development branch in the future

[AlexBVolcy]

@zhongshixi Hey! Just checking in on the status of this PR? I want to be clear that the code coverage comments that I left aren't critical to be implemented for approval as those Shutdown methods are tough to test. Just in case that's what's holding you up. Thank you!

[zhongshixi]

@zhongshixi Hey! Just checking in on the status of this PR? I want to be clear that the code coverage comments that I left aren't critical to be implemented for approval as those Shutdown methods are tough to test. Just in case that's what's holding you up. Thank you!

@AlexBVolcy thanks for the update, i am trying to see if i can really do it without introducing too much hassles, i will give you an update on the PR before EOW.

i have urgent task to deal with this week for my company so sorry for the delay

[zhongshixi]

still working on it

[zhongshixi]

@AlexBVolcy have added some unit test according to your requests, let me know if they look good

[zhongshixi]

cc @bsardo

[guscarreon]

Just like it was done in the AgmaLogger's implementation of Shutdown(), can we please add glog.Infof entries in the FileLogger and PubstackModule implementations?

88   // Shutdown the logger
89   func (f *FileLogger) Shutdown() {
90       // clear all pending buffered data in case there is any
   +     glog.Infof("[FileLogger] Shutdown, trying to flush buffer")
91       f.Logger.Flush()
92   }
analytics/filesystem/file_module.go

and

203   // Shutdown - no op since the analytic module already implements system signal handling
204   // and trying to close a closed channel will cause panic
205 - func (p *PubstackModule) Shutdown() {}
    + func (p *PubstackModule) Shutdown() {
    +     glog.Infof("[PubstackModule] Shutdown")
    + }
analytics/pubstack/pubstack_module.go

[zhongshixi]

glog.Infof("[PubstackModule] Shutdown")

@guscarreon i have updated the PR with your suggestion, take a look

[bsardo]

@zhongshixi sorry I'm coming in a bit late here. I was about to merge this but I noticed that we are introducing another logging library clog. Why do we need this when we already have glog? I suggest we only use one logging library.

[zhongshixi]

@zhongshixi sorry I'm coming in a bit late here. I was about to merge this but I noticed that we are introducing another logging library clog. Why do we need this when we already have glog? I suggest we only use one logging library.

@bsardo i am not introducing a new log, i am simply renaming the existing library github.com/chasex/glog to clog so i can use glog to log the output which stays consistent across other modules.

you could refer to this comment chain for details https://github.com/prebid/prebid-server/pull/3335#discussion_r1638292616

[bsardo]

@zhongshixi sorry I'm coming in a bit late here. I was about to merge this but I noticed that we are introducing another logging library clog. Why do we need this when we already have glog? I suggest we only use one logging library.

@bsardo i am not introducing a new log, i am simply renaming the existing library github.com/chasex/glog to clog so i can use glog to log the output which stays consistent across other modules.

you could refer to this comment chain for details #3335 (comment)

Ah gotcha. Thanks for the clarification.