feat: Aggregating Alerts Feature #2230

jdslaugh · 2024-02-12T18:53:19Z

Description

Aggregates alert notices that are passed into the AlertList component and then parses them in the DefinitionList.

Motivation and Context

The UI is cluttered when there are numerous alerts taking up space, especially when many of them have the same title. Aggregating them into one modal alleviates this and reduces the visual strain on the user.

Screenshots (Before)

Notices

Modal

Info SVG Icon Alignment

Screenshots (After)

Notices

Modal

Storybook

Info SVG Icon Alignment

How Has This Been Tested?

Utilizing mock data to recreate the notices that can be aggregated as well as without aggregation.
Unit testing through already existing tests in place
Verification and updates to storybook and related components

CheckList

PR title addresses the issue accurately and concisely
Updates Documentation and Docstrings
Adds tests
Adds instrumentation (logs, or UI events)

boring-cyborg · 2024-02-12T18:53:24Z

Congratulations on your first Pull Request and welcome to Amundsen community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/amundsen-io/amundsen/blob/main/CONTRIBUTING.md)

kristenarmes

looks good overall! just had one comment that I realized based on your screenshot of the grouped notices

kristenarmes · 2024-02-12T22:55:44Z

frontend/amundsen_application/static/js/components/Alert/AlertList.tsx

+      const { messageHtml, severity, payload } = notice;
+
+      if (payload) {
+        accum[messageHtml] ??= {


I just noticed this in your screenshot in the PR description where it groups the notices with the same message but different severities into 1 - I'm thinking if the severity is different we should group those separately if possible, so by unique message and severity? I don't think that would happen for us in our cases, but not sure if it might for others who use this feature

Ah this is a great point. Yeah I can group them by message + severity, let me get a fix up. Thanks for the find!

Alright, took a few days to get back to this but the changes are now up and screenshots are updated. So it will now group by message and then sub-group by severity, which is then sorted to show up

Golodhros

LGTM

Golodhros · 2024-02-15T17:00:02Z

frontend/amundsen_application/static/js/components/Alert/AlertList.tsx

+    if (notice) {
+      const { messageHtml, severity, payload } = notice;
+
+      /* eslint-disable no-param-reassign */


We could get rid of this rule, we have better things to care about!

Sounds good!

Adjusted the rule to turn it off

Golodhros · 2024-02-15T17:00:11Z

frontend/amundsen_application/static/js/components/Alert/AlertList.tsx

+}
+
+const aggregateNotices = (notices) =>
+  notices.reduce((accum, notice: any) => {


Any way to avoid this 'any'?

Yeah should be able to, might've just been leftover from my first implementation :)

Mapped it to NoticeType

Golodhros · 2024-02-15T17:04:13Z

Maybe I would add a count on the aggregated notices, something like a (X) at the end of the text.

feng-tao · 2024-02-20T01:01:08Z

the feature looks great!

jdslaugh · 2024-02-22T20:51:31Z

Maybe I would add a count on the aggregated notices, something like a (X) at the end of the text.

Ooh that's a good idea, yeah I can do that!

Signed-off-by: Josh Slaughter <[email protected]>

- Sorting of notices by severity - Added a number next to show details if # of notices > 1 - Adjusted css for info-svg-icon to align it with others - Turned off param-reassign rule Signed-off-by: Josh Slaughter <[email protected]>

jdslaugh · 2024-02-23T21:06:33Z

frontend/amundsen_application/static/js/components/Alert/AlertList.tsx

+    if (notice) {
+      const { messageHtml, severity, payload } = notice;
+
+      if (typeof messageHtml !== 'function') {


Only case I'm not sure of is when messageHtml is a function and takes a resourceName. Not really sure what I would use for that case but if anyone has any ideas I can do it or if we assume it's not happening for this we can just ignore it I guess.

good question, I just checked and we have one instance where we were using this functionality at Lyft. I looked into this a bit and I don't think it would ever be a function type at this point. I included my investigation below, but TLDR I think this is fine to leave as you have it here

in the table detail page it passes the results from this function aggregateResourceNotices to the AlertList, and that function calls getResourceNotices. in there it transforms the notices that have function types as their messageHtml to call the function and set set the field to its string message. I investigated backward from the original implementation of function messageHtml fields here

kristenarmes

looks great, thanks for all the updates!

I see there is a failure from betterer as the last thing, you can run npm run betterer under the directory frontend/amundsen_application/static to test locally. I'm not sure what the best solution is for this case bc I think this is what you tried to turn off...if it is too much of a pain to fix, I'm fine with you using the comment like you had before to bypass

Signed-off-by: Josh Slaughter <[email protected]>

boring-cyborg · 2024-02-29T01:49:27Z

Awesome work, congrats on your first merged pull request!

jdslaugh requested review from Golodhros, ttannis, allisonsuarez, feng-tao, verdan, mgorsk1, sewardgw, youngyjd, dechoma, kristenarmes, ozandogrultan and B-T-D as code owners February 12, 2024 18:53

boring-cyborg bot added the area:frontend From the Frontend folder label Feb 12, 2024

jdslaugh force-pushed the aggregated-alerts branch from ed3a3b7 to d54dbe4 Compare February 12, 2024 18:54

jdslaugh closed this Feb 12, 2024

jdslaugh reopened this Feb 12, 2024

kristenarmes reviewed Feb 12, 2024

View reviewed changes

jdslaugh force-pushed the aggregated-alerts branch from fc13bc5 to 7b6cb9a Compare February 13, 2024 19:45

Golodhros approved these changes Feb 15, 2024

View reviewed changes

jdslaugh added 3 commits February 23, 2024 13:02

Aggregating Alerts Feature

43a6a71

Signed-off-by: Josh Slaughter <[email protected]>

Fixing lint issue

5997b3c

Signed-off-by: Josh Slaughter <[email protected]>

jdslaugh force-pushed the aggregated-alerts branch from 3d33552 to a420234 Compare February 23, 2024 21:02

jdslaugh commented Feb 23, 2024

View reviewed changes

kristenarmes approved these changes Feb 27, 2024

View reviewed changes

Adjusting betterer to account for eslint rule change

5ad5ca4

Signed-off-by: Josh Slaughter <[email protected]>

kristenarmes merged commit 73313b8 into amundsen-io:main Feb 29, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Aggregating Alerts Feature #2230

feat: Aggregating Alerts Feature #2230

jdslaugh commented Feb 12, 2024 •

edited

Loading

boring-cyborg bot commented Feb 12, 2024

kristenarmes left a comment

kristenarmes Feb 12, 2024

jdslaugh Feb 14, 2024

jdslaugh Feb 23, 2024

Golodhros left a comment

Golodhros Feb 15, 2024

jdslaugh Feb 22, 2024

jdslaugh Feb 23, 2024

Golodhros Feb 15, 2024

jdslaugh Feb 22, 2024

jdslaugh Feb 23, 2024

Golodhros commented Feb 15, 2024

feng-tao commented Feb 20, 2024

jdslaugh commented Feb 22, 2024

jdslaugh Feb 23, 2024

kristenarmes Feb 27, 2024

kristenarmes left a comment

boring-cyborg bot commented Feb 29, 2024

feat: Aggregating Alerts Feature #2230

feat: Aggregating Alerts Feature #2230

Conversation

jdslaugh commented Feb 12, 2024 • edited Loading

Description

Motivation and Context

Screenshots (Before)

Notices

Modal

Info SVG Icon Alignment

Screenshots (After)

Notices

Modal

Storybook

Info SVG Icon Alignment

How Has This Been Tested?

CheckList

boring-cyborg bot commented Feb 12, 2024

kristenarmes left a 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

Golodhros left a 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

Golodhros commented Feb 15, 2024

feng-tao commented Feb 20, 2024

jdslaugh commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristenarmes left a comment

Choose a reason for hiding this comment

boring-cyborg bot commented Feb 29, 2024

jdslaugh commented Feb 12, 2024 •

edited

Loading