Use batch logic to push logs by mahmednabil109 · Pull Request #129 · linuxboot/contest

mahmednabil109 · 2022-08-15T10:31:59Z

Signed-off-by: Mohamed Abokammer mahmednabil109@gmail.com

codecov-commenter · 2022-08-15T11:16:31Z

Codecov Report

Merging #129 (5b83b81) into main (2ba91cd) will decrease coverage by 0.23%.
The diff coverage is 0.00%.

❗ Current head 5b83b81 differs from pull request most recent head 5119b40. Consider uploading reports for the commit 5119b40 to get more accurate results

@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
- Coverage   63.85%   63.62%   -0.24%     
==========================================
  Files         164      164              
  Lines       10312    10347      +35     
==========================================
- Hits         6585     6583       -2     
- Misses       3009     3040      +31     
- Partials      718      724       +6

Flag	Coverage Δ
e2e	`49.46% <0.00%> (-0.27%)`	⬇️
integration	`55.04% <0.00%> (-0.23%)`	⬇️
unittests	`49.39% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmds/admin_server/server/server.go	`0.00% <0.00%> (ø)`
cmds/admin_server/storage/mongo/mongo.go	`8.19% <0.00%> (+0.34%)`	⬆️
pkg/loggerhook/httphook.go	`0.00% <0.00%> (ø)`
pkg/jobmanager/jobmanager.go	`76.92% <0.00%> (-1.10%)`	⬇️
pkg/runner/step_runner.go	`88.84% <0.00%> (-0.40%)`	⬇️
plugins/teststeps/waitport/waitport.go	`68.93% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rihter007 · 2022-08-15T13:58:16Z


 type Storage interface {
 	StoreLog(ctx xcontext.Context, entry Log) error
+	StoreLogs(ctx xcontext.Context, batch LogBatch) error


why do we need a LogBatch type? Why not to just []Log?

to make it easy to add any metadata -in the futuer- along with the batch.

valid point, but what kind of metadata do you expect?

xaionaro · 2022-08-15T13:58:56Z

+func (lb *LogBatch) ToStorageBatch() storage.LogBatch {
+	var logBatch storage.LogBatch
+	for _, log := range lb.Logs {
+		logBatch.Logs = append(logBatch.Logs, log.ToStorageLog())
+	}
+	return logBatch
+}


Quick question: should this function be thread-safe?

I can't see why, the logBatch will not be shared across threads.

rihter007 · 2022-08-15T14:21:13Z

@@ -169,6 +209,7 @@ func initRouter(ctx xcontext.Context, rh RouteHandler, middlewares []gin.Handler

 	r.GET("/status", rh.status)
 	r.POST("/log", rh.addLog)


I also expected that batching will apply to /log API, so the local server will hold sending the logs further. @mimir-d what do you think?

I can merge the two endpoints, by always sending an array of logs.

It is ok to have two separate, but inside use batch for everything

it's fine to change the endpoint now, but keep in mind that this is a breaking change and wont work after this starts to be used

rihter007 · 2022-08-15T14:22:11Z

+	MaxBatchSize      = 500000 // size in bytes
+	MaxBatchLength    = 100
+	BatchRetries      = 4
+	BatchRetryDelay   = 100 * time.Millisecond


Having good defaults is great, but I would also add overriding with command line parameters

yeah, i guess it will be better to add it in another PR.

will be added, #130

rihter007 · 2022-08-15T14:24:18Z


+func (hh *HttpHook) postBatch() error {
+	logs := hh.logBatch
+	hh.logBatch = make([]server.Log, 0)


in context with retry hh.logBatch will be 0 after first try and if it failed, they will be lost by the time of the second try

rihter007 · 2022-08-15T14:27:57Z

-				fmt.Fprintf(os.Stderr, "Http Logger Err: %v", err)
+			hh.batchMux.Unlock()
+		case <-hh.sendTicker.C:
+			hh.batchMux.Lock()


As far as I understand logHandler works in a single goroutine, so if batch variables are not used elsewhere, having mutexes is not needed.

Also block everything during retries (that have Sleep) is not great, probably a good way is to either launch a goroutine that will try to post or to add retries in the current loop. Probably retrying at the next tick is ok.

rihter007 · 2022-08-15T16:37:04Z

+			if len(hh.logBatch) > MaxBatchLength || hh.batchSize > uint64(MaxBatchSize) {
+				err := hh.postBatch()
+				if err != nil {
+					fmt.Fprintf(os.Stderr, "Send Batch faild: %v", err)


nit

"... failEd: %v"

rihter007 · 2022-08-15T16:37:51Z

+					break
+				}
+				// if the batch is sent
+				hh.logBatch = make([]server.Log, 0)


nit

Just hh.logBatch = nil

rihter007

Overall looks good, just left some final comments

rihter007

`LGTM!

mimir-d

the mandatory change is to address non-empty current batch when closing; rest are recommendations

Signed-off-by: Mohamed Abokammer <mahmednabil109@gmail.com>

mimir-d · 2022-08-16T13:00:53Z


+// Batch defines a log batch that handles the size in bytes of the logs
+type Batch struct {
+	addr string


this isnt realy a property of the batch, but i can ignore this; you shouldve had it as a param in send

mahmednabil109 marked this pull request as draft August 15, 2022 10:32

mahmednabil109 force-pushed the add_batch_log_push branch from 53a63b9 to ba3ccb1 Compare August 15, 2022 10:58

mahmednabil109 force-pushed the add_batch_log_push branch 2 times, most recently from 76852c9 to be60ffb Compare August 15, 2022 13:10

mahmednabil109 marked this pull request as ready for review August 15, 2022 13:33

mimir-d requested a review from rihter007 August 15, 2022 13:57

rihter007 reviewed Aug 15, 2022

View reviewed changes

xaionaro reviewed Aug 15, 2022

View reviewed changes

rihter007 reviewed Aug 15, 2022

View reviewed changes

mahmednabil109 force-pushed the add_batch_log_push branch 2 times, most recently from 6a97c5d to 7f9fa92 Compare August 15, 2022 15:53

rihter007 reviewed Aug 15, 2022

View reviewed changes

Comment thread pkg/loggerhook/httphook.go

rihter007 reviewed Aug 15, 2022

View reviewed changes

Comment thread pkg/loggerhook/httphook.go Outdated

rihter007 previously approved these changes Aug 15, 2022

View reviewed changes

mahmednabil109 dismissed rihter007’s stale review via f49e533 August 15, 2022 16:52

mahmednabil109 force-pushed the add_batch_log_push branch from 7f9fa92 to f49e533 Compare August 15, 2022 16:52

rihter007 previously approved these changes Aug 15, 2022

View reviewed changes

mimir-d requested changes Aug 16, 2022

View reviewed changes

mahmednabil109 dismissed rihter007’s stale review via 7e34e60 August 16, 2022 10:23

mahmednabil109 force-pushed the add_batch_log_push branch from f49e533 to 7e34e60 Compare August 16, 2022 10:23

mimir-d reviewed Aug 16, 2022

View reviewed changes

Comment thread pkg/loggerhook/httphook.go Outdated

Use batch logic to push logs

5119b40

Signed-off-by: Mohamed Abokammer <mahmednabil109@gmail.com>

mahmednabil109 force-pushed the add_batch_log_push branch from 7e34e60 to 5119b40 Compare August 16, 2022 11:38

rihter007 approved these changes Aug 16, 2022

View reviewed changes

mimir-d reviewed Aug 16, 2022

View reviewed changes

mimir-d approved these changes Aug 16, 2022

View reviewed changes

mimir-d merged commit a3f975d into linuxboot:main Aug 16, 2022

		@@ -169,6 +209,7 @@ func initRouter(ctx xcontext.Context, rh RouteHandler, middlewares []gin.Handler

		r.GET("/status", rh.status)
		r.POST("/log", rh.addLog)

Conversation

mahmednabil109 commented Aug 15, 2022

Uh oh!

codecov-commenter commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rihter007 left a comment

Choose a reason for hiding this comment

Uh oh!

rihter007 left a comment

Choose a reason for hiding this comment

Uh oh!

mimir-d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Aug 15, 2022 •

edited

Loading