🪟 🐛 Add DbtCloudErrorBoundary #20616

ambirdsall · 2022-12-17T00:32:47Z

What

Adds an error boundary for the dbt cloud integration. As a follow up, I would like to better test out the error response coming from the server; while I did use the actual JSON structure of a known dbt Cloud error response and code the error parsing defensively (so if it's incorrect it should gracefully fall back to a generic error UI instead of erroring) , all my attempts to actually trigger an error response from dbt Cloud caused downstream errors in the cloud server and thus a 500 response with no forwarding of dbt Cloud's 4XX response payload.

Closes #20441, but not https://github.com/airbytehq/airbyte-cloud/issues/4181

ambirdsall · 2023-01-12T21:43:41Z

@mfsiega-airbyte, I tried loading the transformation tab for an account using a deleted access token, but got a 500 response from the list dbt jobs endpoint. This error shouldn't propagate to the point that it crashes a server thread, it should rather be a 4XX status, ideally including the dbt Cloud API payload so the frontend can display something useful to the user. Not urgent: in the short-term, I'll just clean up this code in order to make the generic message ready to merge and in the medium term I'm going to be pretty focused on free connectors.

mfsiega-airbyte · 2023-01-19T15:53:18Z

@ambirdsall just wanted to check - do we want to try to merge this? We got another similar report today.

tybernstein · 2023-01-24T14:18:33Z

@mfsiega-airbyte @ambirdsall Just bumping this to see if we can proceed with the related OC issue?

ambirdsall · 2023-01-24T16:45:03Z

@TBernstein4 @mfsiega-airbyte I don't think there's any reason to hold off on getting this merged: it would be nice to iterate a little further on the code to parse out dbt-sourced error messages, but even if it shows the generic fallback message 100% of the time that's a clear improvement over the app's current behavior. I'll resolve all conflicts and pull this PR out of draft status so we can ship it soon.

mfsiega-airbyte · 2023-01-24T20:25:56Z

Just to check - what did the validation look like here? (Looks reasonable to me, as long as it Works.)

ambirdsall · 2023-01-25T00:08:00Z

Just to check - what did the validation look like here? (Looks reasonable to me, as long as it Works.)

Here's the steps I took. In order, I:

generated a new service token with job admin privileges
registered it to an airbyte workspace
verified everything worked as expected
deleted the new token in dbt cloud
saw the generic error message in the transformation tab

It certainly could look nicer, but it seems like the first priority should be unbreaking the UX in this scenario, followed by a quick visual iteration with Nico's help:

josephkmh · 2023-01-27T10:43:22Z

...te-webapp/src/pages/connections/ConnectionTransformationPage/DbtCloudTransformationsCard.tsx

@@ -25,6 +25,50 @@ interface DbtJobListValues {
  jobs: DbtCloudJob[];
 }

+const DbtCloudErrorBoundary = class extends React.Component {


This file is getting really full of components - could we split this into a new file to ease readability & file navigation?

@ambirdsall not sure if you intend to address this comment still?

@josephkmh this is what happens when I make a mental note but not a physical one. Great point, totally slipped my mind 🤦 👍

...te-webapp/src/pages/connections/ConnectionTransformationPage/DbtCloudTransformationsCard.tsx

josephkmh

Code LGTM - just one of my comments is still open: #20616 (comment)

Not sure if you want to address that before merging? OK for me either way 👍

ambirdsall · 2023-02-01T01:14:43Z

@josephkmh I split out helper components. The main component has a lot less code to wade through, now.

The scss import lint rule meant I had to restructure the CSS, too, so I recorded a screen capture (using a chrome extension to trigger artificial API errors) to show the actual rendered styles in the different contexts (a 500 response to /get_available_dbt_jobs, an empty jobs list, a nonempty obs list):

josephkmh

Thanks for breaking out those components into separate files! Looks good

It's a bit more efficient and I don't need any of the other things provided by `useCurrentWorkspace`.

I put a little effort into keeping all of the API interactions within the top-level component, and a linting rule required me to split out individual stylesheets for each helper component (plus one non-module scss file for shared card styles that individual scss modules can `@forward`)

octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 17, 2022

ambirdsall changed the title ~~Add WIP DbtCloudErrorBoundary~~ 🪟 🐛 Add DbtCloudErrorBoundary Dec 17, 2022

ambirdsall force-pushed the alex/add-custom-error-boundary-to-dbt-Cloud-transformation-card branch from b4d5163 to 4dad820 Compare January 24, 2023 18:38

ambirdsall marked this pull request as ready for review January 24, 2023 18:38

ambirdsall requested a review from mfsiega-airbyte January 24, 2023 18:38

octavia-squidington-iv removed the area/platform issues related to the platform label Jan 24, 2023

ambirdsall force-pushed the alex/add-custom-error-boundary-to-dbt-Cloud-transformation-card branch from 4dad820 to a60b8cd Compare January 26, 2023 22:29

ambirdsall requested review from teallarson and josephkmh January 26, 2023 22:36

josephkmh requested changes Jan 27, 2023

View reviewed changes

ambirdsall force-pushed the alex/add-custom-error-boundary-to-dbt-Cloud-transformation-card branch 2 times, most recently from a0ad62c to 6ddd82c Compare January 30, 2023 23:34

ambirdsall requested a review from josephkmh January 30, 2023 23:34

josephkmh approved these changes Jan 31, 2023

View reviewed changes

mfsiega-airbyte approved these changes Jan 31, 2023

View reviewed changes

ambirdsall force-pushed the alex/add-custom-error-boundary-to-dbt-Cloud-transformation-card branch from 6ddd82c to 9634479 Compare February 1, 2023 00:45

ambirdsall requested a review from josephkmh February 1, 2023 01:14

josephkmh approved these changes Feb 3, 2023

View reviewed changes

ambirdsall added 4 commits February 6, 2023 15:00

Add WIP DbtCloudErrorBoundary

b4db50e

get workspaceId via useCurrentWorkspaceId

3f6b280

It's a bit more efficient and I don't need any of the other things provided by `useCurrentWorkspace`.

Track dbt Cloud errors via AppMonitoringService

99d89ea

Put available jobs query within error boundary

f10a7ac

ambirdsall force-pushed the alex/add-custom-error-boundary-to-dbt-Cloud-transformation-card branch from 7c038de to f10a7ac Compare February 6, 2023 23:00

ambirdsall enabled auto-merge (squash) February 6, 2023 23:05

ambirdsall merged commit 3ef7edf into master Feb 6, 2023

ambirdsall deleted the alex/add-custom-error-boundary-to-dbt-Cloud-transformation-card branch February 6, 2023 23:27

github-actions bot mentioned this pull request Feb 7, 2023

Bump helm chart version reference to 0.43.28 #22489

Closed

This was referenced Feb 8, 2023

Bump Airbyte version from 0.40.32 to 0.40.33 #22578

Closed

Bump Airbyte version from 0.40.32 to 0.40.33 #22689

Closed

Bump Airbyte version from 0.40.32 to 0.40.33 #22839

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🐛 Add DbtCloudErrorBoundary #20616

🪟 🐛 Add DbtCloudErrorBoundary #20616

ambirdsall commented Dec 17, 2022 •

edited

Loading

ambirdsall commented Jan 12, 2023

mfsiega-airbyte commented Jan 19, 2023

tybernstein commented Jan 24, 2023

ambirdsall commented Jan 24, 2023

mfsiega-airbyte commented Jan 24, 2023

ambirdsall commented Jan 25, 2023 •

edited

Loading

josephkmh Jan 27, 2023

josephkmh Jan 31, 2023

ambirdsall Jan 31, 2023

josephkmh left a comment

ambirdsall commented Feb 1, 2023

josephkmh left a comment •

edited

Loading

🪟 🐛 Add DbtCloudErrorBoundary #20616

🪟 🐛 Add DbtCloudErrorBoundary #20616

Conversation

ambirdsall commented Dec 17, 2022 • edited Loading

What

ambirdsall commented Jan 12, 2023

mfsiega-airbyte commented Jan 19, 2023

tybernstein commented Jan 24, 2023

ambirdsall commented Jan 24, 2023

mfsiega-airbyte commented Jan 24, 2023

ambirdsall commented Jan 25, 2023 • edited Loading

josephkmh Jan 27, 2023

Choose a reason for hiding this comment

josephkmh Jan 31, 2023

Choose a reason for hiding this comment

ambirdsall Jan 31, 2023

Choose a reason for hiding this comment

josephkmh left a comment

Choose a reason for hiding this comment

ambirdsall commented Feb 1, 2023

josephkmh left a comment • edited Loading

Choose a reason for hiding this comment

ambirdsall commented Dec 17, 2022 •

edited

Loading

ambirdsall commented Jan 25, 2023 •

edited

Loading

josephkmh left a comment •

edited

Loading