feat(frontend/ingestion): Support flagged / warning / connection failure statuses; add recipe #8920

asikowitz · 2023-09-28T20:38:08Z

Prepares frontend to be able to render new ingestion statuses. I was going to put these statuses in the metadata model, but that seems kind of committal. I make two changes to the existing statuses on the frontend:

Change cancelled status icon
Adds periods to the status description copy

Also adds a recipe section because I want this all the time when debugging.

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

hsheth2 · 2023-10-02T20:11:04Z

datahub-web-react/src/app/ingest/source/executions/ExecutionRequestDetailsModal.tsx

-    const isOutputExpandable = output.length > 100;
+
+    const recipeJson = data?.executionRequest?.input.arguments?.find((arg) => arg.key === 'recipe')?.value;
+    const recipeYaml = recipeJson && YAML.stringify(JSON.parse(recipeJson), 8, 2);


do we need error handling here? maybe fallback to raw string if json parsing fails?

hsheth2 · 2023-10-02T20:11:43Z

datahub-web-react/src/app/ingest/source/executions/ExecutionRequestDetailsModal.tsx

                            <ShowMoreButton type="link" onClick={() => setShowExpandedLogs(!showExpandedLogs)}>
                                {showExpandedLogs ? 'Hide' : 'Show More'}
                            </ShowMoreButton>
                        )}
                    </Typography.Paragraph>
                </LogsSection>
+                {recipe && (
+                    <RecipeSection>


i'm very excited for this

w00000hoooo

hsheth2 · 2023-10-02T20:13:42Z

datahub-web-react/src/app/ingest/source/utils.ts

        (status === FAILURE && 'red') ||
+        (status === CONNECTION_FAILURE && 'crimson') ||


probably want to refactor this to be a map of status -> {color, text, icon} and then make all of these methods just to a map lookup

Yeah, would be a nice follow-up

hsheth2

LGTM

want to get chris's eyes on this too

hsheth2 · 2023-10-13T17:33:19Z

datahub-web-react/src/app/ingest/source/executions/ExecutionRequestDetailsModal.tsx

+    try {
+        recipeYaml = recipeJson && YAML.stringify(JSON.parse(recipeJson), 8, 2);
+    } catch (e) {
+        recipeYaml = '';


we can also just fall back to

Suggested change

recipeYaml = '';

recipeYaml = recipeJson;

I'd probably want to alter the expanding logic if I supported this, so going to hold off on making this change for simplicity. If this is a common occurrence we can revisit

jjoyce0510 · 2023-10-16T21:40:11Z

datahub-web-react/src/app/ingest/source/utils.ts

@@ -40,7 +43,10 @@ export function getPlaceholderRecipe(ingestionSources: SourceConfig[], type?: st

 export const RUNNING = 'RUNNING';
 export const SUCCESS = 'SUCCESS';
+export const FLAGGED = 'FLAGGED';


what does flagged mean?

jjoyce0510 · 2023-10-16T21:40:38Z

datahub-web-react/src/app/ingest/source/utils.ts

 export const FAILURE = 'FAILURE';
+export const CONNECTION_FAILURE = 'CONNECTION_FAILURE';


really happy to finally see us adding more rich statuses.

we are doing the same in datahub-monitors-service when running a test. we should align on the error types where possible.

datahub-web-react/src/app/ingest/source/utils.ts

chriscollins3456

nice! the code looks solid. as far as UX goes I'd vote for making the recipe section expandable to show or hide the whole section as opposed to always showing a truncated recipe (since some users won't care about the recipe)

I could be overruled though and don't feel too strongly here

chriscollins3456 · 2023-10-17T21:51:23Z

datahub-web-react/src/app/ingest/source/executions/ExecutionRequestDetailsModal.tsx

                            <ShowMoreButton type="link" onClick={() => setShowExpandedLogs(!showExpandedLogs)}>
                                {showExpandedLogs ? 'Hide' : 'Show More'}
                            </ShowMoreButton>
                        )}
                    </Typography.Paragraph>
                </LogsSection>
+                {recipe && (
+                    <RecipeSection>
+                        <SectionHeader level={5}>Recipe</SectionHeader>


I feel like this whole recipe should be hidden by default and expanded only if they care to see it, as opposed to showing a truncated recipe with ... inside of the code block for the recipe itself that you show more or less of - which feels weird itself tbh (even though we do that with logs above it)

maybe we can make it more like the "Advanced" section at the end of defining a recipe with a toggle button to the side of the title "Recipe" (which could remain the same styling as you have now) that would show or hide the whole recipe section, so users who don't care about the recipe don't see it at all

alternatively to hiding the whole section, I'd like to show less of the recipe so that it doesn't show so much right away like i'm seeing in your screenshot (seems like we show 75% of the recipe right away and takes up much more room than the logs part)

I'm happy to do either, just not sure the best way to do the styling... like the > Advanced is pretty clear it's expandable but how to do the same for the recipe section lol.

I am showing exactly 10 lines of the recipe right now. I'll just drop that down to 1-3

much better!

chriscollins3456 · 2023-10-17T21:54:24Z

datahub-web-react/src/app/ingest/source/executions/ExecutionRequestDetailsModal.tsx

+                        <SectionSubHeader>
+                            <SubHeaderParagraph type="secondary">
+                                The recipe used for this ingestion run.
+                            </SubHeaderParagraph>
+                        </SectionSubHeader>
+                        <Typography.Paragraph ellipsis>
+                            <pre>{`${recipe}${!showExpandedRecipe && isRecipeExpandable ? '\n...' : ''}`}</pre>
+                        </Typography.Paragraph>
+                        {isRecipeExpandable && (
+                            <ShowMoreButton type="link" onClick={() => setShowExpandedRecipe((v) => !v)}>
+                                {showExpandedRecipe ? 'Hide' : 'Show More'}
+                            </ShowMoreButton>
+                        )}
+                    </RecipeSection>
+                )}


i'd advocate for bringing this out into its own small component and we could put shared styled components in a shared file or something

Considering the other sections aren't in their own components, and this component isn't really reusable, I'd rather leave this as is. Eventually I think we'll redesign this entire modal so I don't feel as bad about leaving this like this

i mean that's fine, but in general just because other pieces aren't broken out into their own components doesn't mean we can't write cleaner frontend code going forward.

…ure statuses; add recipe

asikowitz requested review from hsheth2, jjoyce0510 and chriscollins3456 September 28, 2023 20:38

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Sep 28, 2023

vercel bot deployed to Preview September 28, 2023 21:21 View deployment

hsheth2 reviewed Oct 2, 2023

View reviewed changes

asikowitz requested a review from hsheth2 October 6, 2023 14:44

vercel bot deployed to Preview October 6, 2023 14:51 View deployment

hsheth2 approved these changes Oct 13, 2023

View reviewed changes

jjoyce0510 reviewed Oct 16, 2023

View reviewed changes

datahub-web-react/src/app/ingest/source/utils.ts Show resolved Hide resolved

asikowitz requested a review from jjoyce0510 October 16, 2023 22:01

vercel bot deployed to Preview October 16, 2023 22:18 View deployment

asikowitz added the release-0.12.0 PRs targeted for Release 0.12.0 label Oct 17, 2023

chriscollins3456 reviewed Oct 17, 2023

View reviewed changes

asikowitz added 4 commits October 17, 2023 18:51

feat(frontend/ingestion): Support flagged / warning / connection fail…

f6c0042

…ure statuses; add recipe

error handling

412f1eb

remove flagged status

cb8efa4

truncate more

f7e20f9

asikowitz force-pushed the ingestion-status-frontend branch from 6c56da4 to f7e20f9 Compare October 17, 2023 23:19

vercel bot deployed to Preview October 17, 2023 23:36 View deployment

asikowitz merged commit b3ac42b into datahub-project:master Oct 18, 2023
37 of 39 checks passed

asikowitz deleted the ingestion-status-frontend branch October 18, 2023 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend/ingestion): Support flagged / warning / connection failure statuses; add recipe #8920

feat(frontend/ingestion): Support flagged / warning / connection failure statuses; add recipe #8920

asikowitz commented Sep 28, 2023 •

edited

Loading

hsheth2 Oct 2, 2023

hsheth2 Oct 2, 2023

jjoyce0510 Oct 16, 2023

hsheth2 Oct 2, 2023

asikowitz Oct 6, 2023

jjoyce0510 Oct 16, 2023

hsheth2 left a comment

hsheth2 Oct 13, 2023

asikowitz Oct 13, 2023

jjoyce0510 Oct 16, 2023

jjoyce0510 Oct 16, 2023

chriscollins3456 left a comment

chriscollins3456 Oct 17, 2023

chriscollins3456 Oct 17, 2023

chriscollins3456 Oct 17, 2023

asikowitz Oct 17, 2023

asikowitz Oct 17, 2023

chriscollins3456 Oct 18, 2023

chriscollins3456 Oct 17, 2023

asikowitz Oct 17, 2023

chriscollins3456 Oct 18, 2023

		(status === FAILURE && 'red') \|\|
		(status === CONNECTION_FAILURE && 'crimson') \|\|

		export const FAILURE = 'FAILURE';
		export const CONNECTION_FAILURE = 'CONNECTION_FAILURE';

feat(frontend/ingestion): Support flagged / warning / connection failure statuses; add recipe #8920

feat(frontend/ingestion): Support flagged / warning / connection failure statuses; add recipe #8920

Conversation

asikowitz commented Sep 28, 2023 • edited Loading

Checklist

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

hsheth2 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

chriscollins3456 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asikowitz commented Sep 28, 2023 •

edited

Loading