Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Aggregating Alerts Feature #2230

Merged
merged 4 commits into from
Feb 29, 2024
Merged

feat: Aggregating Alerts Feature #2230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43a6a71
Aggregating Alerts Feature
jdslaugh Feb 12, 2024
5997b3c
Fixing lint issue
jdslaugh Feb 13, 2024
a420234
- Deeper grouping of notices, now by message then severity
jdslaugh Feb 23, 2024
5ad5ca4
Adjusting betterer to account for eslint rule change
jdslaugh Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 5 additions & 31 deletions frontend/amundsen_application/static/.betterer.results
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ exports[`eslint`] = {
[71, 6, 118, "Visible, non-interactive elements with click handlers must have at least one keyboard listener.", "3051403928"],
[71, 6, 118, "Static HTML elements with event handlers require a role.", "3051403928"]
],
"js/components/EditableText/index.tsx:175217081": [
[82, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
[100, 6, 13, "Do not use setState in componentDidUpdate", "57229240"]
"js/components/EditableText/index.tsx:2590685581": [
[83, 6, 13, "Do not use setState in componentDidUpdate", "57229240"],
[101, 6, 13, "Do not use setState in componentDidUpdate", "57229240"]
],
"js/components/OwnerEditor/index.tsx:2385247435": [
[85, 6, 13, "Do not use setState in componentDidUpdate", "57229240"]
Expand All @@ -35,12 +35,8 @@ exports[`eslint`] = {
[79, 19, 21, "Must use destructuring state assignment", "3239125001"]
],
"js/ducks/dashboard/api/v0.ts:1572663041": [
[20, 2, 4, "Assignment to property of function parameter \'data\'.", "2087377941"],
[43, 28, 104, "Do not nest ternary expressions.", "1163212497"]
],
"js/ducks/issue/api/v0.ts:638633038": [
[45, 8, 19, "Assignment to property of function parameter \'notificationPayload\'.", "3904454342"]
],
"js/ducks/issue/reducer.ts:891877399": [
[141, 6, 56, "Unexpected lexical declaration in case block.", "2031834906"]
],
Expand All @@ -56,20 +52,14 @@ exports[`eslint`] = {
[417, 6, 61, "Unexpected lexical declaration in case block.", "2053236172"]
],
"js/ducks/search/utils.ts:923002554": [
[19, 2, 8, "Assignment to function parameter \'resource\'.", "2131237679"],
[20, 2, 248, "Expected a default case.", "1034339850"]
],
"js/ducks/tableMetadata/api/v0.ts:3333048528": [
[92, 6, 9, "Assignment to property of function parameter \'tableData\'.", "1466754955"],
[141, 23, 2, "Expected to return a value at the end of arrow function.", "5859494"]
],
"js/ducks/tableMetadata/reducer.ts:3935312006": [
[482, 6, 84, "Unexpected lexical declaration in case block.", "114266473"]
],
"js/ducks/utilMethods.ts:1762524729": [
[21, 6, 3, "Assignment to property of function parameter \'obj\'.", "193420290"],
[34, 6, 3, "Assignment to property of function parameter \'obj\'.", "193420290"]
],
"js/features/Breadcrumb/index.tsx:1046428150": [
[69, 6, 141, "Visible, non-interactive elements with click handlers must have at least one keyboard listener.", "1742605206"],
[69, 6, 141, "Static HTML elements with event handlers require a role.", "1742605206"]
Expand All @@ -79,16 +69,6 @@ exports[`eslint`] = {
[116, 6, 36, "Visible, non-interactive elements with click handlers must have at least one keyboard listener.", "3801508926"],
[116, 6, 36, "Static HTML elements with event handlers require a role.", "3801508926"]
],
"js/features/ColumnList/ColumnType/parser.ts:2297011907": [
[59, 6, 10, "Assignment to function parameter \'startIndex\'.", "3807744539"],
[60, 6, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
[84, 10, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
[86, 8, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
[131, 8, 10, "Assignment to function parameter \'startIndex\'.", "3807744539"],
[132, 8, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
[135, 6, 12, "Assignment to function parameter \'currentIndex\'.", "2078922066"],
[178, 4, 10, "Assignment to function parameter \'columnType\'.", "460876587"]
],
"js/features/Footer/index.tsx:3658564998": [
[45, 8, 22, "Must use destructuring props assignment", "1925601400"],
[49, 12, 22, "Must use destructuring props assignment", "1925601400"]
Expand Down Expand Up @@ -141,14 +121,8 @@ exports[`eslint`] = {
[13, 0, 455, "Component should be written as a pure function", "2334877134"],
[15, 35, 20, "Must use destructuring props assignment", "2510284131"]
],
"js/pages/TableDetailPage/ReportTableIssue/index.tsx:1210646415": [
[170, 15, 20, "Script URL is a form of eval.", "3959800777"]
],
"js/pages/TableDetailPage/TableOwnerEditor/index.tsx:2069554136": [
[30, 4, 3, "Assignment to property of function parameter \'obj\'.", "193420290"]
],
"js/utils/owner.ts:2186084439": [
[10, 4, 9, "Assignment to property of function parameter \'resultObj\'.", "1686251499"]
"js/pages/TableDetailPage/ReportTableIssue/index.tsx:2299677182": [
[168, 15, 20, "Script URL is a form of eval.", "3959800777"]
]
}`
};
2 changes: 1 addition & 1 deletion frontend/amundsen_application/static/.betterer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export default {
'no-extra-boolean-cast': 'error',
'no-multi-str': 'error',
'no-nested-ternary': 'error',
'no-param-reassign': 'error',
'no-param-reassign': 'off',
'no-restricted-globals': 'error',
'no-script-url': 'error',
'no-unneeded-ternary': 'error',
Expand Down
3 changes: 3 additions & 0 deletions frontend/amundsen_application/static/js/components/Alert/Alert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ export const Alert: React.FC<AlertProps> = ({
onClick={handleSeeDetails}
>
{OPEN_PAYLOAD_CTA}
{(payload?.descriptions || []).length > 1
? ` (${(payload.descriptions || []).length})`
: ''}
</button>
);
}
Expand Down
57 changes: 48 additions & 9 deletions frontend/amundsen_application/static/js/components/Alert/AlertList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,67 @@

import * as React from 'react';

import { NoticeType } from 'config/config-types';
import { NoticeSeverity, NoticeType } from 'config/config-types';
import { Alert } from './Alert';

export interface AlertListProps {
notices: NoticeType[];
}

export interface AggregatedAlertListProps {
notices: {
[key: string]: NoticeType;
};
}

const aggregateNotices = (notices) =>
notices.reduce((accum, notice: NoticeType) => {
if (notice) {
const { messageHtml, severity, payload } = notice;

if (typeof messageHtml !== 'function') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (payload) {
accum[messageHtml] ??= {};
accum[messageHtml][severity] ??= {
payload: { descriptions: [] },
};
accum[messageHtml][severity].payload.descriptions.push(payload);
} else {
accum[messageHtml] = {
[severity]: { ...notice },
};
}
}
}

return accum;
}, {});

export const AlertList: React.FC<AlertListProps> = ({ notices }) => {
if (!notices.length) {
return null;
}

const aggregated = aggregateNotices(notices);
const NoticeSeverityValues = Object.values(NoticeSeverity);

return (
<div className="alert-list">
{notices.map((notice, idx) => (
<Alert
key={idx}
message={notice.messageHtml}
severity={notice.severity}
payload={notice.payload}
/>
))}
{Object.keys(aggregated).map((notice, idx) =>
Object.keys(aggregated[notice])
.sort(
(a: NoticeSeverity, b: NoticeSeverity) =>
NoticeSeverityValues.indexOf(a) - NoticeSeverityValues.indexOf(b)
)
.map((severity) => (
<Alert
key={idx}
message={notice}
severity={severity as NoticeSeverity}
payload={aggregated[notice][severity].payload}
/>
))
)}
</div>
);
};
16 changes: 16 additions & 0 deletions frontend/amundsen_application/static/js/components/Alert/alert.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,22 @@ const list = [
messageHtml: 'Second alert of the stack',
},
{ severity: NoticeSeverity.ALERT, messageHtml: 'Third alert of the stack' },
{
severity: NoticeSeverity.ALERT,
messageHtml: 'Aggregated alert of the stack',
payload: {
term: 'Test term 1',
description: 'Test description 1',
},
},
{
severity: NoticeSeverity.ALERT,
messageHtml: 'Aggregated alert of the stack',
payload: {
term: 'Test term 2',
description: 'Test description 2',
},
},
];

export const AlertListStory = (): React.ReactNode => (
Expand Down
5 changes: 5 additions & 0 deletions frontend/amundsen_application/static/js/components/Alert/styles.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ $alert-alert-background: #ffe4dd;
margin-right: $spacer-1;
}

.info-svg-icon {
margin-right: $spacer-half;
margin-left: -$spacer-half;
}

.alert-action {
margin: auto 0 auto auto;
}
Expand Down
45 changes: 42 additions & 3 deletions frontend/amundsen_application/static/js/components/DefinitionList/definitionList.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ export const DefinitionListStory = (): React.ReactNode => (
{
term: 'Verity checks',
description:
'1 out of 4 checks failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Ownser</a>)',
'1 out of 4 checks failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Owner</a>)',
},
{
term: 'Failed DAGs',
description:
'1 out of 4 DAGs failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Ownser</a>)',
'1 out of 4 DAGs failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Owner</a>)',
},
]}
/>
Expand All @@ -55,7 +55,46 @@ export const DefinitionListStory = (): React.ReactNode => (
{
term: 'Failed DAGs',
description:
'1 out of 4 DAGs failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Ownser</a>)',
'1 out of 4 DAGs failed (<a href="http://lyft.com">Link</a> | <a href="http://lyft.com">Owner</a>)',
},
]}
/>
</StorySection>
<StorySection title="DefinitionList with aggregated descriptions">
<DefinitionList
definitions={[
{
term: 'Table name',
description: [
{
'Failed Check': 'coco.fact_rides',
Owner: '<a href="http://lyft.com">Owner</a>',
},
{
'Failed Check 2': 'coco.fact_rides',
Owner: 'Just a normal string',
},
],
},
]}
/>
</StorySection>
<StorySection title="DefinitionList with aggregated descriptions and fixed term width">
<DefinitionList
termWidth={200}
definitions={[
{
term: 'Table name',
description: [
{
'Failed Check': 'coco.fact_rides',
Owner: '<a href="http://lyft.com">Owner</a>',
},
{
'Failed Check 2': 'coco.fact_rides',
Owner: 'Just a normal string',
},
],
},
]}
/>
Expand Down
27 changes: 27 additions & 0 deletions frontend/amundsen_application/static/js/components/DefinitionList/index.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,5 +149,32 @@ describe('DefinitionList', () => {
expect(actual).toEqual(expected);
});
});

describe('when passing aggregated descriptions', () => {
it('should render them', () => {
const { wrapper } = setup({
definitions: [
{
term: 'Table name',
description: [
{
'Failed Check': 'coco.fact_rides',
Owner: '<a href="http://lyft.com">Owner</a>',
},
{
'Failed Check 2': 'coco.fact_rides',
Owner: 'Just a normal string',
},
],
},
],
});

const itemGroup = wrapper.find('.definition-list-items-group');

expect(itemGroup.length).toEqual(2);
expect(itemGroup.find('.definition-list-term').length).toEqual(4);
});
});
});
});
81 changes: 62 additions & 19 deletions frontend/amundsen_application/static/js/components/DefinitionList/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,26 +23,69 @@ export interface DefinitionListProps {
export const DefinitionList: React.FC<DefinitionListProps> = ({
definitions,
termWidth,
}) => (
<dl className="definition-list">
{definitions.map(({ term, description }) => (
<div className="definition-list-container" key={term}>
<dt
className="definition-list-term"
style={{ minWidth: termWidth ? `${termWidth}px` : 'auto' }}
>
{term}
</dt>
<dd className="definition-list-definition">
{typeof description === 'string' ? (
<SanitizedHTML html={description} className="definition-text" />
}) => {
const parseDescription = (description) => {
switch (typeof description) {
case 'object':
return (
<>
{Array.isArray(description)
? description.map((item) => {
const items = Object.keys(item).map((key) => (
<div key={key}>
<div
className="definition-list-term"
style={{
minWidth: termWidth ? `${termWidth}px` : 'auto',
}}
>
{key}:
</div>
<div className="definition-list-definition">
{parseDescription(item[key])}
</div>
</div>
));

return (
<div className="definition-list-items-group">{items}</div>
);
})
: description}
</>
);
case 'string':
return <SanitizedHTML html={description} className="definition-text" />;
default:
return description;
}
};

return (
<dl className="definition-list">
{definitions.map(({ term, description }) => (
<div className="definition-list-container" key={term}>
{Array.isArray(description) ? (
<div className="definition-list-inner-container">
{parseDescription(description)}
</div>
) : (
description
<>
<dt
className="definition-list-term"
style={{ minWidth: termWidth ? `${termWidth}px` : 'auto' }}
>
{term}
</dt>
<dd className="definition-list-definition">
{parseDescription(description)}
</dd>
</>
)}
</dd>
</div>
))}
</dl>
);
</div>
))}
</dl>
);
};

export default DefinitionList;
Loading
Loading