create sap error history index by riysaxen-amzn · Pull Request #859 · opensearch-project/security-analytics

riysaxen-amzn · 2024-02-16T21:58:37Z

Description

This PR creates a SAP history index, the index is generated whenever the detector encounters failure scenarios during the create/update flow.
Index name: .opensearch-sap-error-history

    Field     | Type   | Description
    _id       | String |  Random uuid
    detectorId| String | Id of the detector
    exception | String | Exception/error in case of detector creation/update failure
    timestamp | String  | UTC timestamp when the failure happened
    operation | String  | CREATE_DETECTOR/UPDATE_DETECTOR
    logType   | String  | rule Topic
    user      | String  | name of the user

Scope is currently limited to create/update detector flows, can be later on expanded to errors related to correlations and delete detector workflows

Issues Resolved

Presently, Security Analytics lacks a system to record errors associated with detectors, including failure scenarios. This index will act as a crucial resource for troubleshooting, offering insights into the reasons behind detector failures at specific timestamps.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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.

eirsep · 2024-02-16T23:49:45Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java

        }
    }

+    private void createSAPHistoryIndex(IndexDetectorRequest request, String exception) throws IOException {


plz move this logic to separate class. you can chekc rule indices handling for reference

eirsep · 2024-02-16T23:52:35Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java

+            try {
+                createSAPHistoryIndex(request, t.toString());
+            } catch (IOException e) {
+                throw new RuntimeException(e);


plz dont ever throw runtime exceptions.

no need to throw exception at all here imo since this is not a customer facing feature.

plz add error log for failure and continue execution

eirsep · 2024-02-16T23:53:11Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java

+        builder.field("user", user);
+        builder.endObject();
+        log.info("Index name is: {}", indexName);
+        IndexRequest indexRequest = new IndexRequest(indexName)


this method is not even creating an index. plz rename method correcty.

Creates or updates a document in an index. It also creates an index if it doesn't already exists.

eirsep

plz add mapping for system index.
maintain separate class for system index management. use the right index settings at creation time by referring other system indices' behaviour

riysaxen-amzn · 2024-02-17T00:47:40Z

plz add mapping for system index.

maintain separate class for system index management. use the right index settings at creation time by referring other system indices' behaviour

So, the idea was to create this index whenever we encounter any failure and then upserting it with failure docs. Since this will be an Ops feature, i don't see the utility of creating this index at the beginning. But I see the point since this is a system index it not intended for direct access or modification. So, having a proper system index management for this make sense.

codecov · 2024-02-17T00:58:21Z

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (e3362f6) 24.83% compared to head (fcc53be) 24.75%.
Report is 2 commits behind head on main.

❗ Current head fcc53be differs from pull request most recent head 844c2f9. Consider uploading reports for the commit 844c2f9 to get more accurate results

Files	Patch %	Lines
...lytics/transport/TransportIndexDetectorAction.java	0.00%	37 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #859      +/-   ##
============================================
- Coverage     24.83%   24.75%   -0.08%     
  Complexity     1030     1030              
============================================
  Files           277      277              
  Lines         12713    12754      +41     
  Branches       1401     1404       +3     
============================================
  Hits           3157     3157              
- Misses         9292     9333      +41     
  Partials        264      264

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

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

…-project#859) Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> (cherry picked from commit fcdcdcbf13a6df32693fb7b063c2819c6a80f85e) Co-authored-by: Ashish Agrawal <ashisagr@amazon.com>

engechas · 2024-04-10T00:51:38Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java

-import org.opensearch.securityanalytics.util.RuleTopicIndices;
-import org.opensearch.securityanalytics.util.SecurityAnalyticsException;
-import org.opensearch.securityanalytics.util.WorkflowService;
+import org.opensearch.securityanalytics.util.*;


Nit: we should avoid wildcard imports

engechas · 2024-04-10T00:52:52Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java

        }

        private void onFailures(Exception t) {
+            log.info("Exception failures while creating detector: {}", t.toString());


Let's log this as error level. Same comment for 1529. We can then avoid the t.toString() with just

log.error("Exception failures while creating detector", t);

engechas · 2024-04-10T00:54:48Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java


+    private void initErrorHistoryIndexAndAddErrors(IndexDetectorRequest request, String exceptionMessage) throws IOException {
+
+        if (!errorHistoryIndex.errorHistoryIndexExists()) {


Take it or leave it - negating conditions is a bit harder to read than testing for the non-negated condition first. In a case like this where the method is an 'either or', I find it's easiest to read as

private void method() { if (condition) { // logic return; } // logic for !condition }

engechas · 2024-04-10T00:55:45Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java

+                }
+                @Override
+                public void onFailure(Exception ex) {
+                    log.debug("Exception while creating error history index: {}", ex);


debug -> error

engechas · 2024-04-10T00:56:04Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java

+                            }
+                            @Override
+                            public void onFailure(Exception ex) {
+                                log.info("Exception while inserting a document in error history index: {}", ex);


info -> error

engechas · 2024-04-10T00:56:22Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java

+                }
+                @Override
+                public void onFailure(Exception e) {
+                    log.debug("Exception while creating error history index: {}", e);


debug -> error

engechas · 2024-04-10T00:57:54Z

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java

+                @Override
+                public void onResponse(CreateIndexResponse response) {
+                        errorHistoryIndex.onCreateMappingsResponse(response);
+                    try {


Style nit: the pattern of nesting these action listeners is tough to read imo. This is far from the most flagrant use of it in the codebase but I would still advocate for a private method to handle the try/catch and inner logic. In this case it would also reduce duplication with the else condition below

engechas · 2024-04-10T00:59:58Z

src/main/java/org/opensearch/securityanalytics/util/ErrorsHistoryIndex.java

+            builder.field("user", user);
+            builder.endObject();
+            IndexRequest indexRequest = new IndexRequest(indexName)
+                    .id(UUIDs.base64UUID())


Can we let OpenSearch generate the ID rather than creating a UUID?

riysaxen-amzn requested review from AWSHurneyt, amsiglan, eirsep, engechas, getsaurabh02, jowg-amazon, lezzago, praveensameneni and sbcd90 as code owners February 16, 2024 21:58

riysaxen-amzn force-pushed the addSAPErrorHistoryIndex branch from a2a430b to fcc53be Compare February 16, 2024 22:13

eirsep reviewed Feb 16, 2024

View reviewed changes

eirsep requested changes Feb 16, 2024

View reviewed changes

praveensameneni previously approved these changes Feb 17, 2024

View reviewed changes

riysaxen-amzn dismissed praveensameneni’s stale review via 5c94aaa February 17, 2024 01:02

riysaxen-amzn force-pushed the addSAPErrorHistoryIndex branch 3 times, most recently from 45e8bd4 to d0c9e3d Compare February 17, 2024 04:14

create sap error history index

844c2f9

Signed-off-by: Riya Saxena <riysaxen@amazon.com>

riysaxen-amzn force-pushed the addSAPErrorHistoryIndex branch from d0c9e3d to 844c2f9 Compare February 17, 2024 04:19

riysaxen-amzn requested review from eirsep and praveensameneni February 17, 2024 04:23

engechas reviewed Apr 10, 2024

View reviewed changes


		private void initErrorHistoryIndexAndAddErrors(IndexDetectorRequest request, String exceptionMessage) throws IOException {

		if (!errorHistoryIndex.errorHistoryIndexExists()) {

Conversation

riysaxen-amzn commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

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!

eirsep left a comment

Choose a reason for hiding this comment

Uh oh!

riysaxen-amzn commented Feb 17, 2024

Uh oh!

codecov bot commented Feb 17, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

riysaxen-amzn commented Feb 16, 2024 •

edited

Loading

codecov bot commented Feb 17, 2024 •

edited

Loading