Spawning multiple goroutine for Notification and Sharing http client #1504
Conversation
…multiple goroutine for Notification and Sharing http client
| wg := new(sync.WaitGroup) | ||
| wg.Add(len(integrations)) | ||
| for _, integrationRow := range integrations { | ||
| switch integrationRow.Resource { | ||
| case utils.ScanTypeDetectedNode[utils.NEO4J_VULNERABILITY_SCAN]: | ||
| processIntegration[model.Vulnerability](msg, integrationRow) | ||
| case utils.ScanTypeDetectedNode[utils.NEO4J_SECRET_SCAN]: | ||
| processIntegration[model.Secret](msg, integrationRow) | ||
| case utils.ScanTypeDetectedNode[utils.NEO4J_MALWARE_SCAN]: | ||
| processIntegration[model.Malware](msg, integrationRow) | ||
| case utils.ScanTypeDetectedNode[utils.NEO4J_COMPLIANCE_SCAN]: | ||
| processIntegration[model.Compliance](msg, integrationRow) | ||
| // cloud compliance scans | ||
| integrationRow.Resource = utils.ScanTypeDetectedNode[utils.NEO4J_CLOUD_COMPLIANCE_SCAN] | ||
| processIntegration[model.CloudCompliance](msg, integrationRow) | ||
| } | ||
| go processIntegrationRow(wg, integrationRow, msg) |
There was a problem hiding this comment.
I am not sure we need to do this at all.
What is this solving?
There was a problem hiding this comment.
Instead if len(integrations) number of go routines we can start limited number of them like 5
There was a problem hiding this comment.
I would go one step further and limit to 1, removing go routine entirely ;)
Unless we have a good reason, here it feels we are making the code more complex for little benefit, we are actually stressing the DB access & memory usage more than necessary.
Instead we should optimize the sending of HTTP requests (batch + go routines)
There was a problem hiding this comment.
ok that should minimize frequent neo4j access
|
I have found a couple of issues (memory leak, broken batching) in the current code base. |
| // Send out bytes in buffer immediately if the limit exceeded after adding this event | ||
| if buffer.Len()+len(jsonBytes) > MaxContentLength { | ||
| s.sendRequest(buffer) | ||
| fmt.Println("Splunk sent buffer:") | ||
| fmt.Println(buffer.String()) | ||
| buffer.Reset() | ||
| } | ||
|
|
||
| // Check the response status code | ||
| if resp.StatusCode != http.StatusOK { | ||
| fmt.Println("Failed to send data to Splunk", resp.Status) | ||
| continue | ||
| } | ||
| resp.Body.Close() | ||
| } | ||
| if buffer.Len() > 0 { | ||
| s.sendRequest(buffer) | ||
| } |
There was a problem hiding this comment.
The logic here is hard to follow - we should have only one sendRequest call.
Also, we are losing the jsonBytes data entirely. I suggest we use this generic batcher I am adding:
#1516
This fixes the case when there are all integrations configured and few were not getting processed.
Fixes #
https://github.com/deepfence/enterprise-roadmap/issues/1943
Changes proposed in this pull request: