contextualize findings with additional metadata fields #1899
Conversation
Signed-off-by: Subhobrata Dey <[email protected]>
sbcd90
requested review from
AWSHurneyt,
amsiglan,
bowenlan-amzn,
eirsep,
engechas,
getsaurabh02,
goyamegh,
jowg-amazon,
lezzago,
praveensameneni,
rishabhmaurya and
riysaxen-amzn
as code owners
|
|
||
| // Before the "|" is the doc id and after the "|" is the index | ||
| val docIndex = it.key.split("|") | ||
| val additionalFields = this.fetchDocForFinding( |
There was a problem hiding this comment.
Making a search call per doc is not going to scale.
We have the document in memory as part of the monitor execution. Can we add these additional fields when we fetch that document and then carry them forward from memory?
There was a problem hiding this comment.
@engechas findings are sparse
IMO we should do a second search in bulk for all finding-generating docs
| return response.hits | ||
| } | ||
|
|
||
| private suspend fun fetchDocForFinding( |
There was a problem hiding this comment.
plz add debug and info logs
time taken for search request
There was a problem hiding this comment.
fetch all docs for findings in single call
| } | ||
| } | ||
| val response: SearchResponse = client.suspendUntil { client.search(request, it) } | ||
| if (response.status() !== RestStatus.OK) { |
| .fetchSource(false) | ||
| ) | ||
|
|
||
| if (fields.isNotEmpty()) { |
There was a problem hiding this comment.
this check should be preliminary to assert that if its empty this search is useless
| } | ||
|
|
||
| val additionalFields: MutableMap<String, List<Any>> = mutableMapOf() | ||
| for (field in response.hits.hits[0].fields) { |
There was a problem hiding this comment.
check for empty hits
this is a walking ArrayOutOfBoundsException.
| assertEquals(found.get(), false) | ||
| } | ||
|
|
||
| fun `test execute monitor with dryrun with finding metadata enabled`() { |
There was a problem hiding this comment.
why dry run
plz add more test cases
There was a problem hiding this comment.
add test where additional fields are invalid. additoinal fields list is empty.
| val additionalFields = this.fetchDocForFinding( | ||
| docIndex[1], | ||
| docIndex[0], | ||
| monitor.metadataForFindings!! |
There was a problem hiding this comment.
do we need version check here for serde?
what happens in upgrade or blue green scenarios?
3 participants
Description
contextualize findings with additional metadata fields
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.