Skip to content

displaying the backend error message to the Alerting Page#858

Open
riysaxen-amzn wants to merge 2 commits intoopensearch-project:mainfrom
riysaxen-amzn:fix-errorResponse
Open

displaying the backend error message to the Alerting Page#858
riysaxen-amzn wants to merge 2 commits intoopensearch-project:mainfrom
riysaxen-amzn:fix-errorResponse

Conversation

@riysaxen-amzn
Copy link
Collaborator

Description

The PR incorporates the backend response that the /alerts API returns for alerts in state of error and display the error message on the Alerts page of Security Analytics.

Change:

  • When the state of Alerts is ERROR, With this PR, the error link will now be clickable, leading to the display of a modal. The modal will have backend errorResponse. [img attached for ref]

Image 1-22-24 at 6 35 PM

Issues Resolved

[List any issues this PR will resolve]

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.

Copy link
Member

@eirsep eirsep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you test out that the error is rendered correctly for 5-6 other error scenarios also?

One example:
AlertingException[[RrS6PI0BudBWHJcvaeqR-metadata-RbS6PI0BudBWHJcvZ-og-metadata]: version conflict, required seqNo [28], primary term [1]. current document has seqNo [29] and primary term [1]]; nested: Exception[org.opensearch.index.engine.VersionConflictEngineException: [RrS6PI0BudBWHJcvaeqR-metadata-RbS6PI0BudBWHJcvZ-og-metadata]: version conflict, required seqNo [28], primary term [1]. current document has seqNo [29] and primary term [1]]
@praveensameneni @sbcd90 you can add more errors if required

@codecov
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.99%. Comparing base (890c486) to head (e40c8ef).

❗ Current head e40c8ef differs from pull request most recent head 0aa1251. Consider uploading reports for the commit 0aa1251 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #858      +/-   ##
==========================================
- Coverage   31.34%   30.99%   -0.35%     
==========================================
  Files         159      151       -8     
  Lines        5258     5123     -135     
  Branches      977      896      -81     
==========================================
- Hits         1648     1588      -60     
+ Misses       3436     3345      -91     
- Partials      174      190      +16     

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

@eirsep eirsep self-requested a review January 24, 2024 20:52
Copy link
Member

@eirsep eirsep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz update your error alert doc with below message and verify what is error being shown
AlertingException[[RrS6PI0BudBWHJcvaeqR-metadata-RbS6PI0BudBWHJcvZ-og-metadata]: version conflict, required seqNo [28], primary term [1]. current document has seqNo [29] and primary term [1]]; nested: Exception[org.opensearch.index.engine.VersionConflictEngineException: [RrS6PI0BudBWHJcvaeqR-metadata-RbS6PI0BudBWHJcvZ-og-metadata]: version conflict, required seqNo [28], primary term [1]. current document has seqNo [29] and primary term [1]]

@riysaxen-amzn
Copy link
Collaborator Author

riysaxen-amzn commented Jan 26, 2024

Plz update your error alert doc with below message and verify what is error being shown AlertingException[[RrS6PI0BudBWHJcvaeqR-metadata-RbS6PI0BudBWHJcvZ-og-metadata]: version conflict, required seqNo [28], primary term [1]. current document has seqNo [29] and primary term [1]]; nested: Exception[org.opensearch.index.engine.VersionConflictEngineException: [RrS6PI0BudBWHJcvaeqR-metadata-RbS6PI0BudBWHJcvZ-og-metadata]: version conflict, required seqNo [28], primary term [1]. current document has seqNo [29] and primary term [1]]

Attached screenshot here
Image 1-22-24 at 12 07 PM

Comment on lines +293 to +301
// Function to show the tooltip
showTooltip = (errorMessage) => {
this.setState({ isTooltipVisible: true, errorMessage });
};

// Function to hide the tooltip
hideTooltip = () => {
this.setState({ isTooltipVisible: false });
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please make sure that we are not exposing any sensitive information with this change. Its a good practice to sanitize the error messages before exposing them to the customers.
  2. One recommendation would be to expose only customer-actionable errors, like 4xx for example. Providing internal errors might confuse the users.

@riysaxen-amzn riysaxen-amzn requested a review from eirsep February 14, 2024 18:32

<EuiModalBody>
<EuiIcon type="alert" size="l" />
<p>{alertItem.error_message}</p>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the error_message is null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the error_message is null?

when the status is ERROR, the error_message ideally shouldn't be null but I will check this in the backend, or we can add another null check here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be an empty string now

Signed-off-by: Riya Saxena <riysaxen@amazon.com>
Signed-off-by: Riya <69919272+riysaxen-amzn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants