Error classification UI #1257
Error classification UI #1257
Conversation
GnsP
force-pushed
the
error-classification-ui
branch
2 times, most recently
from
0658689 to
15844a8
Compare
| top: 100%; | ||
| left: 0; | ||
| right: 0; | ||
| background: #ffefef; |
There was a problem hiding this comment.
Should there be any theming/standard color palette?
|
|
||
| useEffect(() => { | ||
| if (!errorDetails && !loading && currentRun?.status === 'FAILED') { | ||
| fetchErrorDetails(); |
There was a problem hiding this comment.
Will this result in multiple or dangling subscriptions if multiple errors occur while the component is present?
There was a problem hiding this comment.
Nope, it should not result in dangling subscriptions, because:
- The fetch function runs only when the error details (for a failed pipeline) has not been fetched before and not being fetched currently.
- The fetch function puts the data (which is always same for the same runid, given that the run has completed) in the redux store. So, it does not make sense for the fetch to be aborted when the component unmounts. If the component unmounts before the data is fetched, the error details should still be fetched and on the subsequent mounting of the component, data should be taken from the redux store instead of calling the API again.
There was a problem hiding this comment.
However, in the original implementation, the loading state was maintained in the component. I have moved this state up, to the redux store.
app/cdap/utils/time.ts
Outdated
| * the License. | ||
| */ | ||
|
|
||
| export function sleep(durationInMs: number): Promise<void> { |
There was a problem hiding this comment.
I don't know if sleep is the best name here - sleep implies that nothing is happening. delay or wait might be better names.
GnsP
force-pushed
the
error-classification-ui
branch
4 times, most recently
from
f57da9c to
c86a972
Compare
| @@ -43,6 +43,10 @@ const ConnectedRunNumWarnings = connect(mapStateToProps)(RunNumWarnings); | |||
| const ConnectedRunNumErrors = connect(mapStateToProps)(RunNumErrors); | |||
|
|
|||
| export default function RunLevelInfo() { | |||
| // error and warning counts are disabled when error classification is available, | |||
| // i.e. post v6.11.0 | |||
| const shouldShowErrorAndWarningCounts = false; | |||
There was a problem hiding this comment.
What's the condition that enables this flag? I see that it is always false now which means Warnings & Error options will be not be shown. Is that expected ?
There was a problem hiding this comment.
Yes, it's expected. We have moved the error and warning counts to logviewer in this PR. Ideally I would have removed this block of code entirely.
I have not removed the old components from the codebase, as the runLevelInfo panel is likely to be rewritten and I wanted to use the old code as reference.
| ) : ( | ||
| <p /> | ||
| )} | ||
| <LogsButtonsContaner> |
| endIcon={<GetAppIcon />} | ||
| data-testid={getDataTestid(`${TEST_PREFIX}.downloadLogsButton`)} | ||
| > | ||
| {T.translate(`${PREFIX}.downloadLogsButton`)} |
There was a problem hiding this comment.
Could you please update the screenshot in the PR's description reflecting latest changes?
There was a problem hiding this comment.
Updated the screenshot
GnsP
force-pushed
the
error-classification-ui
branch
from
c86a972 to
9b0944f
Compare
| return ( | ||
| <div className={classes.root} data-cy="log-viewer-top-panel" data-testid="log-viewer-top-panel"> | ||
| <div className={classes.leftContainer}> | ||
| <RunLogsStatsChips /> |
There was a problem hiding this comment.
Where and when this component is shown? Can you add a screenshot for this also please?
| <React.Fragment> | ||
| <ConnectedRunNumWarnings /> | ||
| <ConnectedRunNumErrors /> | ||
| </React.Fragment> |
There was a problem hiding this comment.
Current change-set will not show error snack bar if the error classification api response is empty, also we are removing Error and Warning info as per above code. Then what is the indication of error for end user when a pipeline run fails and classification api response is empty?
There was a problem hiding this comment.
Have accounted for this in the latest set of changes. If the error classification response is an empty list, we display the error count from metrics in the snackbar and the CTA in the snackbar opens the log viewer.
There was a problem hiding this comment.
nit: can you please add a screenshot for this too?
There was a problem hiding this comment.
Update: after another discussion, we decided not to use the error count from metrics in the snackbar, as this value does not represent the actual number of errors / exceptions. So, in this specific case, the message is "Pipeline execution failed. [VIEW LOGS]"
Added the screenshot.
GnsP
force-pushed
the
error-classification-ui
branch
2 times, most recently
from
104c198 to
c236a86
Compare
GnsP
force-pushed
the
error-classification-ui
branch
from
c236a86 to
2e3b563
Compare
UI components for error classification
Description
Adds support for classifying errors in pipeline execution and highlight errored stages.
PR Type
Links
Jira: CDAP-21100
Test Plan
e2e tests to be added, when support for error classification in hydrator plugins is added.
currently it's infeasible to add e2e tests for this feature that can run in the gh-runner envirinment.
Screenshots