feat(ui): Add support for structured reporting of warnings and failures in the UI ingestion flow (ingest uplift 2/2) #10790
Conversation
…teps' into jj--refactor-post-ingestion-run-steps
…teps' into jj--refactor-post-ingestion-run-steps
|
Warning Rate limit exceeded@jjoyce0510 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 4 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis update enhances how ingestion status is determined and displayed within DataHub by introducing structured reports for better analysis of execution requests. Key changes include importing a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- datahub-web-react/src/app/ingest/source/IngestionSourceTable.tsx (2 hunks)
- datahub-web-react/src/app/ingest/source/executions/ExecutionRequestDetailsModal.tsx (5 hunks)
- datahub-web-react/src/app/ingest/source/executions/IngestionExecutionTable.tsx (2 hunks)
- datahub-web-react/src/app/ingest/source/executions/reporting/StructuredReport.tsx (1 hunks)
- datahub-web-react/src/app/ingest/source/executions/reporting/StructuredReportItem.tsx (1 hunks)
- datahub-web-react/src/app/ingest/source/executions/reporting/StructuredReportItemContext.tsx (1 hunks)
- datahub-web-react/src/app/ingest/source/executions/reporting/StructuredReportItemList.tsx (1 hunks)
- datahub-web-react/src/app/ingest/source/types.ts (1 hunks)
- datahub-web-react/src/app/ingest/source/utils.ts (7 hunks)
- datahub-web-react/src/app/shared/ShowMoreSection.tsx (1 hunks)
- datahub-web-react/src/graphql/ingestion.graphql (2 hunks)
Additional context used
Biome
datahub-web-react/src/app/shared/ShowMoreSection.tsx
[error] 28-28: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
datahub-web-react/src/app/ingest/source/executions/reporting/StructuredReportItemContext.tsx
[error] 39-39: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
Additional comments not posted (14)
datahub-web-react/src/app/shared/ShowMoreSection.tsx (1)
5-15: Styling for ShowMoreButton looks good.The styling is consistent and uses theme constants, which is good for maintainability.
datahub-web-react/src/app/ingest/source/executions/reporting/StructuredReportItemList.tsx (1)
20-41: Good use of React state and conditional rendering.The implementation of visible items and the conditional rendering of the 'Show More' section is well-handled. The use of keys in the map function is correctly implemented to ensure efficient updates.
datahub-web-react/src/app/ingest/source/executions/reporting/StructuredReport.tsx (1)
23-43: Efficient filtering and conditional rendering of report items.The component efficiently filters and conditionally renders report items based on their level. The use of separate components for different levels enhances modularity.
datahub-web-react/src/app/ingest/source/executions/reporting/StructuredReportItem.tsx (1)
59-81: Well-structured component with good use of external context.The component is well-structured and makes good use of the
StructuredReportItemContextfor displaying additional details. The use of a dynamic icon and styled components adds to the flexibility and aesthetic of the component.datahub-web-react/src/graphql/ingestion.graphql (2)
42-45: Addition ofstructuredReportfield inlistIngestionSourcesquery.The addition of the
structuredReportfield allows fetching structured error and warning information related to ingestion sources. Ensure that the backend appropriately supports this field and that it is properly documented.Verification successful
Backend support for
structuredReportfield verified.The
structuredReportfield is indeed supported in the backend, as evidenced by its implementation and usage in multiple TypeScript files.
datahub-web-react/src/app/ingest/source/utils.tsdatahub-web-react/src/app/ingest/source/executions/ExecutionRequestDetailsModal.tsxdatahub-web-react/src/app/ingest/source/builder/RecipeForm/TestConnection/TestConnectionButton.tsxScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the backend support for `structuredReport` field. # Test: Search for the `structuredReport` resolver implementation. rg --type graphql "structuredReport"Length of output: 68
Script:
#!/bin/bash # Description: Verify the backend support for `structuredReport` field. # Test: Search for the `structuredReport` resolver implementation in TypeScript files. rg "structuredReport" --type tsLength of output: 2250
93-96: Addition ofstructuredReportfield ingetIngestionSourcequery.Similar to the previous query, this addition enhances the detail level of ingestion source reporting. This should be checked for backend support and integration with the front-end components that utilize this data.
Verification successful
Backend support for
structuredReportfield ingetIngestionSourcequery is confirmed.The
structuredReportfield is handled in the backend, specifically in theIngestionResolverUtils.javafile, which confirms that this addition is supported.
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ingest/IngestionResolverUtils.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the backend support for `structuredReport` field in the `getIngestionSource` query. # Test: Search for the `structuredReport` resolver implementation. rg --type graphql "structuredReport"Length of output: 68
Script:
#!/bin/bash # Description: Verify the backend support for `structuredReport` field in the `getIngestionSource` query. # Find all .graphql files and search for the term 'structuredReport' within them. fd --extension graphql --exec rg "structuredReport" {}Length of output: 206
Script:
#!/bin/bash # Description: Verify the backend support for `structuredReport` field in the `getIngestionSource` query. # Find all .graphql files and search for the term 'structuredReport' within them, including file names and line numbers for context. fd --extension graphql --exec rg -n -C 5 "structuredReport" {}Length of output: 1265
Script:
#!/bin/bash # Description: Verify the backend support for `structuredReport` field in the `getIngestionSource` query. # Search for the implementation of the `structuredReport` resolver in the backend code. rg "structuredReport" --type java rg "structuredReport" --type typescriptLength of output: 1153
datahub-web-react/src/app/ingest/source/IngestionSourceTable.tsx (1)
6-6: Refactor to usegetIngestionSourceStatusfor status determination.This change is consistent with the previous file, aiming to centralize status determination logic. Verify that
getIngestionSourceStatushandles all edge cases and integrates well with the UI components that display these statuses.Also applies to: 126-126
datahub-web-react/src/app/ingest/source/types.ts (1)
1-195: Comprehensive addition of types and enums for structured reporting.The new enums and interfaces are well-structured and cater to the needs of structured reporting within the application. Ensure that these types are used consistently across the application and that any potential naming conflicts with existing types are resolved.
datahub-web-react/src/app/ingest/source/executions/ExecutionRequestDetailsModal.tsx (1)
16-17: Enhancements to include structured reporting in execution details.The integration of structured reporting via the
StructuredReportcomponent and related utility functions enhances the detail level of execution request insights. Verify that these components and functions are well-integrated and perform as expected in various scenarios.Also applies to: 21-22, 132-132, 143-148, 152-152, 191-191
Verification successful
Enhancements to include structured reporting in execution details are verified.
The
StructuredReportcomponent and related utility functions (getIngestionSourceStatus,getStructuredReport) are well-integrated within the codebase. They appear in multiple relevant files, includingExecutionRequestDetailsModal.tsx, and are used consistently.
StructuredReportis imported and used inExecutionRequestDetailsModal.tsxand other files under thereportingdirectory.getIngestionSourceStatusandgetStructuredReportare utilized inExecutionRequestDetailsModal.tsxand other related files, ensuring their functionality is integrated into the execution details.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of structured reporting components. # Test: Search for integration tests involving `StructuredReport` and related utility functions. rg --type javascript "StructuredReport"Length of output: 74
Script:
#!/bin/bash # Description: Verify integration of structured reporting components and utility functions. # Test: Search for occurrences of `StructuredReport`, `getIngestionSourceStatus`, and `getStructuredReport` in TypeScript files. rg --type typescript "StructuredReport" rg --type typescript "getIngestionSourceStatus" rg --type typescript "getStructuredReport"Length of output: 235
Script:
#!/bin/bash # Description: Verify integration of structured reporting components and utility functions. # Test: Search for occurrences of `StructuredReport`, `getIngestionSourceStatus`, and `getStructuredReport` in .tsx and .ts files. rg --glob '*.tsx' "StructuredReport" rg --glob '*.ts' "StructuredReport" rg --glob '*.tsx' "getIngestionSourceStatus" rg --glob '*.ts' "getIngestionSourceStatus" rg --glob '*.tsx' "getStructuredReport" rg --glob '*.ts' "getStructuredReport"Length of output: 13853
datahub-web-react/src/app/ingest/source/utils.ts (5)
11-11: Approved: Import and constant additions are essential for new features.The imports and constants added are crucial for supporting the new structured error reporting and the 'Succeeded with Warnings' status. These changes align well with the PR objectives.
Also applies to: 52-52, 71-71, 86-86, 103-104, 124-124
148-187: Approved: New utility functions for structured reporting are well-implemented.The functions for mapping and creating structured reports are crucial for the new error reporting feature. They are implemented efficiently and handle various scenarios, including legacy data formats.
Suggestion for Improvement: Graceful handling of unknown types.
[REFACTOR_SUGGESTion]
Consider providing a default message or logging a warning when encountering unknown types, which could aid in debugging and maintenance.- return StructuredReportItemType.UNKNOWN ? rawType : STRUCTURED_REPORT_ITEM_TYPE_TO_DETAILS.get(stdType)?.message; + return StructuredReportItemType.UNKNOWN ? `Unknown type: ${rawType}` : STRUCTURED_REPORT_ITEM_TYPE_TO_DETAILS.get(stdType)?.message;Also applies to: 189-249
269-287: Approved: Enhanced status handling ingetIngestionSourceStatus.The modifications to
getIngestionSourceStatusare crucial for supporting the new 'Succeeded with Warnings' status. This change allows the UI to more accurately reflect the state of an ingestion process.Suggestion for Improvement: Enhance documentation for clarity.
Adding more detailed comments explaining why the status might be overridden could help future maintainers understand the decision logic more clearly.
+ // Check if the ingestion completed successfully but with warnings, and adjust the status accordingly. if (status === SUCCESS && (structuredReport?.warnCount || 0) > 0) { return SUCCEEDED_WITH_WARNINGS; }
189-249: Approved: Effective handling of structured and legacy report formats intransformToStructuredReport.This function is well-designed to accommodate both new structured report data and legacy formats, ensuring that the application can handle data from various versions seamlessly. This is crucial for maintaining functionality across different deployment states.
Line range hint
23-50: Review of remaining elements: No action needed.The remaining parts of the file, including some utility functions and constants, are unchanged in this PR and continue to serve their existing purposes without modifications.
Also applies to: 53-70, 87-102, 125-147, 252-268
datahub-web-react/src/app/ingest/source/executions/reporting/StructuredReportItemContext.tsx
Outdated
Show resolved
Hide resolved
| @@ -4,7 +4,7 @@ import styled from 'styled-components'; | |||
| import { StyledTable } from '../../../entity/shared/components/styled/StyledTable'; | |||
| import { ExecutionRequest } from '../../../../types.generated'; | |||
| import { ButtonsColumn, SourceColumn, StatusColumn, TimeColumn } from './IngestionExecutionTableColumns'; | |||
| import { SUCCESS } from '../utils'; | |||
| import { SUCCESS, getIngestionSourceStatus } from '../utils'; | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Add test cases for getIngestionSourceStatus.
The function getIngestionSourceStatus is used in multiple places in the codebase, but there are no test cases found for it. To ensure reliability and maintainability, please add comprehensive test cases for this function.
- Files using
getIngestionSourceStatus:datahub-web-react/src/app/ingest/source/IngestionSourceTable.tsxdatahub-web-react/src/app/ingest/source/executions/IngestionExecutionTable.tsxdatahub-web-react/src/app/ingest/source/executions/ExecutionRequestDetailsModal.tsxdatahub-web-react/src/app/ingest/source/utils.ts
Analysis chain
Refactor to use getIngestionSourceStatus for status determination.
The change centralizes the logic for determining the status of ingestion sources, which is a good practice for maintainability. Ensure that the getIngestionSourceStatus function is thoroughly tested to handle all possible statuses correctly.
Also applies to: 115-115
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the correct usage and test coverage of `getIngestionSourceStatus`.
# Test: Search for the test cases for `getIngestionSourceStatus`.
rg --type javascript "getIngestionSourceStatus"
Length of output: 82
Script:
#!/bin/bash
# Description: Verify the correct usage and test coverage of `getIngestionSourceStatus`.
# Search for the usage of `getIngestionSourceStatus` in TypeScript and TSX files.
rg --type-add 'tsx:*.{ts,tsx}' "getIngestionSourceStatus"
# Search for test cases for `getIngestionSourceStatus` in TypeScript and TSX test files.
rg --type-add 'tsx:*.{ts,tsx}' "getIngestionSourceStatus" --glob '*.{test,spec}.{ts,tsx}'
Length of output: 1107
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
jjoyce0510
changed the title
…es in the UI ingestion flow (ingest uplift 2/2) (#10790) Co-authored-by: John Joyce <john@Johns-MBP.lan> Co-authored-by: John Joyce <john@ip-192-168-1-200.us-west-2.compute.internal> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…es in the UI ingestion flow (ingest uplift 2/2) (datahub-project#10790) Co-authored-by: John Joyce <john@Johns-MBP.lan> Co-authored-by: John Joyce <john@ip-192-168-1-200.us-west-2.compute.internal> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
This PR is the first step in structured error reporting visible to the admin via the UI. In this PR, we add a way for visualizing failure and warnings from the ingestion run directly inside the UI, on the ingestion results modal.
We add base support for some error types. We still need to do the proper mapping, so for now most warnings are handled in the default manner shown below, with the following details:
Ideally, over time we'll migrate ingestion to start reporting better formed errors. I've also added support pre-emptively for a new format for failure and warning reporting:
We also add a new "status" type called "Succeeded with Warnings" which is automatically displayed when there are more than 1 warnings reported by the ingestion source during the run!
Once this is merged, we will be incentivized to improve error reporting in ingestion to give the user actionable reports.
Status
Ready for review
Screenshots
Checklist
Summary by CodeRabbit
New Features
StructuredReportcomponent.Improvements
GraphQL
listIngestionSourcesandgetIngestionSourcequeries to include structured report information.