change in findingsAPI call to accommodate backend findings enhancement #859

riysaxen-amzn · 2024-01-25T01:45:15Z

Description

Earlier the findings page only shows the latest 10000 findings per detector. So, if there are 10 detectors there will be latest 100000 findings shown in the UI. With this enhancement, findings page will have the latest 100000 findings for all the detectors collectively or irrespective of all the detectors.
The above change will reflect on Findings ad Overview Page
Backend PR: get all findings as part of findings API enhancement security-analytics#803

Issues Resolved

opensearch-project/security-analytics#795

Check List

Commits are signed per the DCO using --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.

codecov · 2024-01-25T18:20:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7837a99) 31.03% compared to head (265c6fe) 30.96%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #859      +/-   ##
==========================================
- Coverage   31.03%   30.96%   -0.07%     
==========================================
  Files         151      151              
  Lines        5111     5119       +8     
  Branches      948      896      -52     
==========================================
- Hits         1586     1585       -1     
- Misses       3335     3344       +9     
  Partials      190      190

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AWSHurneyt · 2024-01-25T18:23:55Z

public/pages/Findings/containers/Findings/Findings.tsx

+              detector: detector,
+            });
+          });
+          const detectorFindingsPromises = findingRes.response.findings.map(async (finding) => {


Does this need to be async? On line 161, we await the response of the getFindings API call, so I don't believe we'll be dealing with promises here (correct me if I'm missing something though).

yes right, it does not necessarily need to be async because we're working with the result of an already awaited API call (findingsService.getFindings()).

AWSHurneyt · 2024-01-25T18:26:03Z

public/pages/Findings/containers/Findings/Findings.tsx

+                detectorName: detectorInfo ? detectorInfo.name : 'Unknown',
+                logType: detectorInfo ? detectorInfo.logType : 'Unknown',


Nitpick: Would it make sense to use the DEFAULT_EMPTY_DATA constant (link) here instead of Unknown?

Also, do you happen to know if there are instances when we'd expect the logType or detectorName to be undefined for a finding? If not, I'm wondering if it would be worthwhile to add a log of some kind here as that seems like it might be an odd anomaly.

AWSHurneyt · 2024-01-25T18:35:14Z

public/pages/Findings/containers/Findings/Findings.tsx

-          }
+          });
+
+          const detectorFindings = await Promise.all(detectorFindingsPromises);


Similar to the question I had on line 180, are we sure these are promises? I believe we're already awaiting the response from the API calls on lines 161 and 168, but I may have missed something.

Yes, the await keyword is required in this context. The Promise.all function returns a single promise that resolves to an array of the fulfilled values of the input promises (or rejects with the reason of the first promise that rejects, if any). In order to wait for all the promises inside detectorFindingsPromises to resolve before moving on to the next line of code, we are using await with Promise.all.

AWSHurneyt · 2024-01-25T18:41:37Z

public/pages/Findings/containers/Findings/Findings.tsx

@@ -252,7 +264,7 @@ class Findings extends Component<FindingsProps, FindingsState> {
  generateVisualizationSpec() {
    const visData: FindingVisualizationData[] = [];

-    this.state.filteredFindings.forEach((finding: FindingItemType) => {
+    this.state.filteredFindings.forEach((finding: any) => {


Does the FindingItemType no longer align with the schema for the findings that are returned? If so, would it be possible to update the FindingItemType to accommodate the new structure, or perhaps make a new type? Using type any can make understanding the code a little harder.

AWSHurneyt · 2024-01-25T18:44:40Z

public/services/FindingsService.ts

  ): Promise<ServerResponse<GetFindingsResponse>> => {
-    const { detectorType, detectorId } = detectorParams;
+    const { detectorType, detectorId } = detectorParams || {};


Nitpick: Rather than declaring detectorParams as a nullable parameter, I believe you could just refactor line 19 to detectorParams: GetFindingsParams = {} to provide a default value when the parameter is null. This would then eliminate the need for the || {} at the end of line 21.

Signed-off-by: Riya Saxena <[email protected]>

amsiglan · 2024-01-28T14:19:01Z

@riysaxen-amzn I don't see any change that increases the count of findings being fetched to 100000. Also, isn't there a new from parameter that we will be using to actually paginate the results in the frontend? The changes in this PR will not meet our aim of paginating the results and showing all the findings present in the index irrespective of the detectors

amsiglan · 2024-01-28T13:32:51Z

public/pages/Findings/containers/Findings/Findings.tsx

+              detector: detector,
+            });
+          });
+          const detectorFindingsPromises = findingRes.response.findings.map((finding) => {


We are not returning promises in the map function, so we don't need to use Promises.all below

amsiglan · 2024-01-28T13:33:24Z

public/pages/Findings/containers/Findings/Findings.tsx

+          findings = findings.concat(detectorFindings.filter(Boolean));
+          // Set the state after all asynchronous operations are completed
+          await this.getRules(Array.from(ruleIds));
+          this.setState({ findings });


We were setting detector's state as well previously, I think we missed it here

amsiglan · 2024-01-28T14:10:11Z

public/pages/Findings/containers/Findings/Findings.tsx

+            detectorInfoMap.set(detector._id, {
+              logType: detector._source.detector_type,
+              name: detector._source.name,
+              detector: detector,


Can be simplified to just detector

amsiglan · 2024-01-28T14:12:38Z

public/pages/Findings/containers/Findings/Findings.tsx

+              // Use detectorInfoMap to get detector information
+              const detectorInfo = detectorInfoMap.get(finding.detectorId);
+              finding.queries.forEach((rule) => ruleIds.add(rule.id));
+              return {


Let's simplify this to set the detector related info before the return so we have a single check for detectorInfo

const detectorData = detectorInfo ? { detectorName: detectorInfo.name ... } : { detectorName: DEFAULT.... }

riysaxen-amzn requested review from amsiglan, AWSHurneyt, getsaurabh02, lezzago, praveensameneni, sbcd90 and eirsep as code owners January 25, 2024 01:45

AWSHurneyt reviewed Jan 25, 2024

View reviewed changes

change in findingsAPI call to accomodate backend findingd enhancement

265c6fe

Signed-off-by: Riya Saxena <[email protected]>

riysaxen-amzn force-pushed the fix-allFindings branch from ee40f67 to 265c6fe Compare January 28, 2024 01:55

riysaxen-amzn requested a review from AWSHurneyt January 28, 2024 01:56

amsiglan requested changes Jan 28, 2024

View reviewed changes

amsiglan closed this Mar 13, 2024

riysaxen-amzn deleted the fix-allFindings branch April 5, 2024 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change in findingsAPI call to accommodate backend findings enhancement #859

change in findingsAPI call to accommodate backend findings enhancement #859

riysaxen-amzn commented Jan 25, 2024 •

edited

Loading

codecov bot commented Jan 25, 2024 •

edited

Loading

AWSHurneyt Jan 25, 2024

riysaxen-amzn Jan 28, 2024

AWSHurneyt Jan 25, 2024

AWSHurneyt Jan 25, 2024

AWSHurneyt Jan 25, 2024

riysaxen-amzn Jan 28, 2024

AWSHurneyt Jan 25, 2024

AWSHurneyt Jan 25, 2024

amsiglan commented Jan 28, 2024

amsiglan Jan 28, 2024

amsiglan Jan 28, 2024

amsiglan Jan 28, 2024

amsiglan Jan 28, 2024

		detectorName: detectorInfo ? detectorInfo.name : 'Unknown',
		logType: detectorInfo ? detectorInfo.logType : 'Unknown',

change in findingsAPI call to accommodate backend findings enhancement #859

change in findingsAPI call to accommodate backend findings enhancement #859

Conversation

riysaxen-amzn commented Jan 25, 2024 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Jan 25, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amsiglan commented Jan 28, 2024

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

riysaxen-amzn commented Jan 25, 2024 •

edited

Loading

codecov bot commented Jan 25, 2024 •

edited

Loading