-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Display additional failure information when sync is expanded and jump to relevant log line #12896
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. removing myself as a reviewer since I am no good for reviewing UI code
airbyte-webapp/src/components/JobItem/components/FailureDetails.tsx
Outdated
Show resolved
Hide resolved
0f7d1f3
to
d379334
Compare
d379334
to
a807c0a
Compare
Co-authored-by: Tim Roes <[email protected]>
Co-authored-by: Tim Roes <[email protected]>
Co-authored-by: Tim Roes <[email protected]>
Co-authored-by: Tim Roes <[email protected]>
cc49aa3
to
27efa7b
Compare
ok @timroes we are now just showing the (properly formatted) failure timestamp and not scrolling. I think this is ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks great. Tested locally.
scrollToLine={undefined} | ||
highlight={[]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a hack to prevent it from scrolling to a specific line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep - and it was undocumented and super hard to find. That's why I left this in so we would remember this for the future when we get to #12128
@@ -47,8 +46,10 @@ const Logs: React.FC<LogsProps> = ({ logsArray }) => { | |||
lineClassName="logLine" | |||
highlightLineClassName="highlightLogLine" | |||
selectableLines | |||
follow | |||
follow={true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we prefer it without the ={true}
Fun fact - I had to retry this flaky test 5x before it passed https://github.com/airbytehq/airbyte/actions/runs/2392338287 |
… to relevant log line (airbytehq#12896) * Display addtional failure information when sync is expanded * rename * Update airbyte-webapp/src/components/JobItem/components/FailureDetails.tsx Co-authored-by: Tim Roes <[email protected]> * fix bad merge * jump to timestamp * cleat timestamp when logs colapsed * speed up search * rename * Do what @pedroslopez says * Rebase from master after big API update * `floor` matchtimes for greater range matching * Update airbyte-webapp/src/components/JobItem/components/ErrorDetails.tsx Co-authored-by: Tim Roes <[email protected]> * Update airbyte-webapp/src/components/JobItem/components/ErrorDetails.tsx Co-authored-by: Tim Roes <[email protected]> * Update airbyte-webapp/src/components/JobItem/components/Logs.tsx Co-authored-by: Tim Roes <[email protected]> * Update airbyte-webapp/src/components/JobItem/JobItem.tsx Co-authored-by: Tim Roes <[email protected]> * replace regexp with `dayJs` * Extract dayjs into globals module * mach time in full-second resolution * revert `dayJs` and use `Date.parse` * Just show failure timestamp rather than scroll Co-authored-by: Tim Roes <[email protected]>
When viewing your list of syncs, if there is an
internalMessage
on the failure, this PR will display that message to the user when the sync is expanded:If there is a timestamp for that failure reason, you can now also choose to jump to the first line in the logs that matches that timestamp.
Screen.Recording.2022-05-18.at.2.21.49.PM.mov
As the platform gets better at tracking errors (e.g. #12423, there will be both a user-facing "externalMessage" and more detailed "internalMessage") about each failure. This PR displays both to the user.
Alternatives considered:
Closes #12628 and #12130