Gracefully shutdown analytics module/runner #3335

zhongshixi · 2023-12-05T15:12:30Z

Problem

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

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

AlexBVolcy · 2023-12-11T22:07:46Z

analytics/build/build.go

+func (ea enabledAnalytics) Shutdown() {
+	for _, module := range ea {
+		module.Shutdown()
+	}
+}


This code block isn't covered by tests, can you update this?

AlexBVolcy · 2023-12-11T22:09:34Z

router/router.go


 	analyticsRunner := analyticsBuild.New(&cfg.Analytics)

+	// todo(zachbadgett): better shutdown


Can this TODO be removed?

i did not remove this because i do not have context of TODO - is it okay to remove this line ? @zachbadgett

i will just remove it - i think i figured out what is a better shutdown

AlexBVolcy · 2023-12-11T22:10:30Z

router/router.go

+	r.Shutdown = func() {
+		shutdown()
+		analyticsRunner.Shutdown()
+	}


Is it possible to cover this code with tests? I know a lot of code in this file is uncovered already, so no worries if not.

let me see what i can do

it might work, but requires a big refactoring effort, as the router itself is not fully dependency inverted - that means i have to figure out how to pass a proper mock to test this function early.

ideally, i think we should apply dependency inversion pattern to the initialization process of the router to make it easier to test.

cc @bsardo
let me know if you think it is necessary as part of the PR to start invert analytic runner and storedRequestsConf.NewStoredRequests or we should do full dependency inversion as the prebid server 3.0 ( i am all in refactoring the code for better design pattern )

guscarreon · 2023-12-16T00:24:23Z

analytics/filesystem/file_module.go

@@ -85,6 +85,9 @@ func (f *FileLogger) LogNotificationEventObject(ne *analytics.NotificationEvent)
 	f.Logger.Flush()
 }

+// Shutdown the logger - No-op Implementation
+func (f *FileLogger) Shutdown() {}


Should we call flush in order to output all pending buffered data just in case there's any?

88 // Shutdown the logger - No-op Implementation 89 - func (f *FileLogger) Shutdown() {} + func (f *FileLogger) Shutdown() { + f.Logger.Flush() + } 90 91 // Method to initialize the analytic module 92 func NewFileLogger(filename string) (analytics.Module, error) { analytics/filesystem/file_module.go

guscarreon · 2023-12-16T00:45:30Z

analytics/pubstack/pubstack_module.go

@@ -200,6 +200,8 @@ func (p *PubstackModule) LogAmpObject(ao *analytics.AmpObject) {
 	p.eventChannels[amp].Push(payload)
 }

+func (p *PubstackModule) Shutdown() {}


Should we closeAllEventChannels() or close(p.stopCh)?

202 203 - func (p *PubstackModule) Shutdown() {} + func (p *PubstackModule) Shutdown() { + } + p.closeAllEventChannels() + close(p.stopCh) + } 204 205 func (p *PubstackModule) start(c <-chan *Configuration) { analytics/pubstack/pubstack_module.go

@guscarreon after inspection, i would say, leave it no-op, see the comments

hhhjort · 2024-03-13T17:15:37Z

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

zhongshixi · 2024-03-21T17:53:18Z

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

zhongshixi · 2024-04-09T15:01:22Z

going to resume the work today

AlexBVolcy · 2024-04-10T18:58:34Z

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

zhongshixi · 2024-04-10T20:19:39Z

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 · 2024-04-11T02:20:48Z

analytics/agma/agma_module.go

@@ -261,6 +261,11 @@ func (l *AgmaLogger) LogVideoObject(event *analytics.VideoObject) {
 	l.bufferCh <- data
 }

+func (l *AgmaLogger) Shutdown() {


Is it possible to add tests to cover these lines

@AlexBVolcy updated the unit test for it

AlexBVolcy · 2024-04-11T02:22:32Z

analytics/filesystem/file_module.go

@@ -85,6 +85,12 @@ func (f *FileLogger) LogNotificationEventObject(ne *analytics.NotificationEvent)
 	f.Logger.Flush()
 }

+// Shutdown the logger
+func (f *FileLogger) Shutdown() {


Can you add test to cover these line?

@AlexBVolcy updated unit test for it

guscarreon · 2024-04-12T20:38:18Z

analytics/filesystem/file_module.go

@@ -85,6 +85,12 @@ func (f *FileLogger) LogNotificationEventObject(ne *analytics.NotificationEvent)
 	f.Logger.Flush()
 }

+// Shutdown the logger
+func (f *FileLogger) Shutdown() {
+	// clear all pending buffered data in case there is any


Just to follow the same pattern of the agma module, should we glog.Infof?

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

there is no additional args to template the log message, so glog.Info is sufficient.

AlexBVolcy · 2024-04-23T20:09:12Z

@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 · 2024-04-24T14:07:12Z

@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 · 2024-05-03T14:13:45Z

still working on it

zhongshixi · 2024-05-03T17:28:19Z

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

zhongshixi · 2024-05-13T18:20:43Z

cc @bsardo

AlexBVolcy · 2024-05-14T14:44:01Z

analytics/filesystem/file_module_test.go

+func (mw *MockLogger) Debug(v ...interface{}) {
+	mw.Called(v)
+}
+
+func (mw *MockLogger) Flush() {


Nitpick: Could you update the name of this variable to be ml?

guscarreon · 2024-05-21T20:48:19Z

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 · 2024-05-22T17:56:55Z

glog.Infof("[PubstackModule] Shutdown")

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

guscarreon · 2024-06-13T14:11:15Z

analytics/filesystem/file_module.go

@@ -93,6 +94,7 @@ func (f *FileLogger) LogNotificationEventObject(ne *analytics.NotificationEvent)
 // Shutdown the logger
 func (f *FileLogger) Shutdown() {
 	// clear all pending buffered data in case there is any
+	gglog.Info("[FileLogger] Shutdown, trying to flush buffer")


Nitpick: can we reference this library as glog as it's done in other parts of the code?

guscarreon · 2024-06-13T14:11:23Z

analytics/filesystem/file_module.go

@@ -5,6 +5,7 @@ import (
 	"fmt"

 	"github.com/chasex/glog"
+	gglog "github.com/golang/glog"


Nitpick: can we reference this library as glog as it's done in other parts of the code?

we have two glogs here

the code uses two glogs and the existing implementation uses glog to refer github.com/chasex/glog

github.com/chasex/glog github.com/golang/glog

if we use glog to refer to github.com/golang/glog , then we have to name "github.com/chasex/glog" to gglog or other proper name.

let me know your thought on it

It seems like github.com/chasex/glog last commit was 8 years ago and we should probably move on to use github.com/golang/glog. If it gets complicated and it's too far out of the scope of this PR, I'm ready to approve. If we don't fix it, it'd be desirable to create an issue for later fix. Thank you for your updates @zhongshixi

i agree, i do not think it is actually complicated. let me change it so you can take a another look

@guscarreon take another look, i aliased github.com/chasex/glog as clog so i do not change the critical implementation, the module now uses glog to output log information

guscarreon

LGTM

bsardo · 2024-07-22T14:13:25Z

@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 · 2024-07-25T20:13:10Z

@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)

bsardo · 2024-07-26T17:07:58Z

@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.

zhongshixi changed the title ~~graceful shutdown of the analytic module/runner~~ graceful shutdown for the analytic module/runner Dec 5, 2023

bsardo assigned guscarreon and AlexBVolcy Dec 5, 2023

bsardo added the do not port label Dec 6, 2023

bsardo changed the title ~~graceful shutdown for the analytic module/runner~~ Gracefully shutdown analytics module/runner Dec 6, 2023

AlexBVolcy reviewed Dec 11, 2023

View reviewed changes

guscarreon reviewed Dec 16, 2023

View reviewed changes

zhongshixi added 2 commits April 9, 2024 14:54

introduce graceful shutdown of the analytic module/runner

a5f43ce

rebase to the latest master

4452580

zhongshixi force-pushed the analytic-adapter-graceful-shutdown-interface branch from 1f0b6b0 to 4452580 Compare April 9, 2024 18:56

zhongshixi added 3 commits April 9, 2024 16:50

refactor code according to the review

e558c26

remove duplicate os.Interrupt since we already have syscall.SIGTERM

9651ffc

refactor code

179ff41

zhongshixi requested review from AlexBVolcy and guscarreon April 9, 2024 21:28

zhongshixi added 2 commits April 10, 2024 22:03

add glog message

d3374fb

remove empty space

0c135fb

AlexBVolcy reviewed Apr 11, 2024

View reviewed changes

guscarreon reviewed Apr 12, 2024

View reviewed changes

zhongshixi added 2 commits May 3, 2024 11:42

Merge branch 'master' into analytic-adapter-graceful-shutdown-interface

33a4a16

added unit test

1c75891

zhongshixi requested review from AlexBVolcy and guscarreon May 3, 2024 17:28

AlexBVolcy reviewed May 14, 2024

View reviewed changes

zhongshixi added 2 commits May 17, 2024 00:16

refactor the name

ed06458

Merge branch 'master' into analytic-adapter-graceful-shutdown-interface

f9e25ff

AlexBVolcy previously approved these changes May 21, 2024

View reviewed changes

added glog info on shutdown

55b6592

zhongshixi dismissed AlexBVolcy’s stale review via 55b6592 May 22, 2024 17:56

Merge branch 'master' into analytic-adapter-graceful-shutdown-interface

74833f5

AlexBVolcy previously approved these changes May 23, 2024

View reviewed changes

guscarreon reviewed Jun 13, 2024

View reviewed changes

zhongshixi added 2 commits July 3, 2024 10:08

Merge branch 'master' into analytic-adapter-graceful-shutdown-interface

47d39df

refactor the code

e1083fe

zhongshixi dismissed AlexBVolcy’s stale review via e1083fe July 3, 2024 14:11

zhongshixi requested review from AlexBVolcy and guscarreon July 5, 2024 13:56

guscarreon approved these changes Jul 9, 2024

View reviewed changes

AlexBVolcy approved these changes Jul 10, 2024

View reviewed changes

bsardo merged commit ed3e4a1 into prebid:master Jul 29, 2024
3 checks passed


		analyticsRunner := analyticsBuild.New(&cfg.Analytics)

		// todo(zachbadgett): better shutdown

Gracefully shutdown analytics module/runner #3335

Gracefully shutdown analytics module/runner #3335

Conversation

zhongshixi commented Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongshixi Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongshixi Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort commented Mar 13, 2024

zhongshixi commented Mar 21, 2024 • edited Loading

zhongshixi commented Apr 9, 2024

AlexBVolcy commented Apr 10, 2024

zhongshixi commented Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy commented Apr 23, 2024

zhongshixi commented Apr 24, 2024

zhongshixi commented May 3, 2024

zhongshixi commented May 3, 2024

zhongshixi commented May 13, 2024

AlexBVolcy May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon commented May 21, 2024

zhongshixi commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

zhongshixi Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

bsardo commented Jul 22, 2024

zhongshixi commented Jul 25, 2024

bsardo commented Jul 26, 2024

zhongshixi commented Dec 5, 2023 •

edited

Loading

zhongshixi Apr 9, 2024 •

edited

Loading

zhongshixi Apr 9, 2024 •

edited

Loading

zhongshixi commented Mar 21, 2024 •

edited

Loading

zhongshixi commented Apr 10, 2024 •

edited

Loading

AlexBVolcy May 14, 2024 •

edited

Loading

guscarreon Jun 19, 2024 •

edited

Loading

zhongshixi Jun 21, 2024 •

edited

Loading