Skip to content
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

Show update log on update error #1307

Closed
mtlynch opened this issue Feb 9, 2023 · 6 comments
Closed

Show update log on update error #1307

mtlynch opened this issue Feb 9, 2023 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@mtlynch
Copy link
Collaborator

mtlynch commented Feb 9, 2023

When the update fails, we give the user a pretty opaque error message:

image

Meanwhile, in the error logs, we have useful information:

ERROR: Failed building wheel for cryptography
Failed to build cryptography
ERROR: Could not build wheels for cryptography, which is required to install pyproject.toml-based projects

https://logs.tinypilotkvm.com/kRbcpZt0

We should show the user the end of the update log and give the user the opportunity to easily capture the full logs at this point, since we really want to prevent update failures.

@mtlynch mtlynch added the enhancement New feature or request label Feb 9, 2023
@jotaen4tinypilot jotaen4tinypilot self-assigned this Mar 16, 2023
@jotaen4tinypilot
Copy link
Contributor

@mtlynch A few thoughts on how we could approach this. I’d appreciate your feedback, maybe we manage to settle on a direction. Otherwise, I’m also happy to think more about this.

  • In the <update-dialog>, we have the full update log at our disposal when the error occurs, so it should be fairly easy to include the full text in the “Details” box (or only the tail of it). I think that would be worthwhile to do in any event.
  • At some point, we abandoned the option for users to copy the entire log output the clipboard. I think the reasoning behind this is still valid. The update logs might be equally long, depending on the point at which the error occurs in the update process, so I’m not sure it makes sense to bring back a “copy error to clipboard” button in the error dialog. I think we should only do this if we make sure that the “Details” won’t exceed a certain length, like say 20 lines – so in case of the update error, only if we show the truncated text.
  • The “canonical” way for users to share error logs would be via a shareable link. That’s how we do it in the <debug-dialog>, at least. Based on that, we could consider to…:
    • Resemble the functionality from the <debug-dialog> in the <error-dialog>. In contrast to the <debug-dialog>, however, it would only share the specific error, without all the other system logs. We could achieve that by placing a “Get Shareable URL” link above the “Details” box, that would transition the dialog to basically the same state as after sharing from the <debug-dialog>.
      Screenshot 2023-03-16 at 19 21 13
    • Offer a button that takes the user to the system log dialog (<debug-dialog>). From there, they could view and share the full system logs as usual, and they also would be in full control over whether to hide sensitive data in the shared log text. Note, that we could also extend the wording of the dialog message in this case, so that the user knows what to make out of that “Open System Logs” button.
      Screenshot 2023-03-16 at 19 42 31

For both “sharing” options, we could either offer these new buttons/functionality by default for all errors, or we make it “opt in”, so that only particular errors have these extra buttons.

We could also combine the approaches, e.g.:

  • Show truncated update logs in “Details” box for update errors
  • Always have “Copy Error to Clipboard” button in <error-dialog> (as the error messages should usually be short)
  • Offer “Open System Logs” button at the bottom, to make further diagnosis easier

@mtlynch
Copy link
Collaborator Author

mtlynch commented Mar 16, 2023

At some point, #960. I think the reasoning behind this is still valid.

Ah, right! I forgot about that. Yeah, that's still true. I don't want to invite users to dump huge pastes anywhere, so I think shareable URL is the better option.

Resemble the functionality from the in the . In contrast to the , however, it would only share the specific error, without all the other system logs. We could achieve that by placing a “Get Shareable URL” link above the “Details” box, that would transition the dialog to basically the same state as after sharing from the .

Yeah, I like this one. I think System Logs is a good idea, but if something went wrong, I don't want to send the user to another screen without context. I'd rather they feel like they have everything they need to share diagnostics here.

I don't think there's a need to add it to other error dialogs yet, but we can keep it in mind if we encounter others where we expect that there's error messages we want to make it easier for the user to share.

@jotaen4tinypilot
Copy link
Contributor

jotaen4tinypilot commented Mar 20, 2023

Proposal

Based on our discussion above, I’d like to propose the following UI changes, which aim to make the “Get Shareable URL” feature from the logs dialog (<debug-dialog> reusable in the error dialog.

Screen.Recording.2023-03-20.at.11.35.40.mov

Discussion

The problem with our current UI is that it transitions the dialog into a separate state for showing the resulting URL. That state is a dead end, however (because you can’t go back to the previous state), and it also lacks all the original context in which the user requested the URL.

For the <debug-dialog>, that is not that big of a problem, because the user is able to re-open the logs dialog again via “System > Logs”. For the <error-dialog>, however, there is no way to go back to the original error message, e.g. to make a screenshot, or review the initial error message again.

One approach to solve this would be to offer a “Back” button, but that would not just be tricky to implement with our current internal control flows (which are loosely coupled via events), I also would find that a bit “inelegant”. To me, a better solution would be to fetch the “shareable URL” without transitioning the dialog to a separate state, and then display the URL in-place once it’s there. That way, the original context is retained, and the action is less intrusive. (Which seems appropriate, since the action is secondary.)

We also wouldn’t need to show the conditional reminder that “sensitive data was hidden”, since the respective toggle button is literally right next to the “Get Shareable URL” button. (You could say that the reason why we introduced that reminder hint was said loss of context.)

Task break-down

  • Introduce new standalone component <share-logs-button>, that contains both the UI functionality and the communication with the logs server
  • Replace existing “Get Shareable URL” button in <debug-dialog> with that new <share-logs-button>
  • Add new <share-logs-button> to <error-dialog> component in a conditional way, and activate it for update errors

@mtlynch
Copy link
Collaborator Author

mtlynch commented Mar 20, 2023

Cool, yeah I like that design a lot!

jotaen4tinypilot added a commit that referenced this issue Mar 28, 2023
Part of #1307.

This PR introduces a new frontend component `<share-logs-button>`, that
allows us to re-use the logs sharing functionality in other contexts as
well, e.g. for sharing error logs (from the `<error-dialog>`).

The look and usage is demonstrated in
#1336.

A few notes:

- I expanded the comment in `clipboard.js` a bit. I also realized that
we might have to replace this workaround, as `document.execCommand` is
deprecated. For now, we should still be good, though.
- I parametrized the colors for the `<progress-spinner>`, to adjust the
contrast to fit the disabled “action” button. I’m not sure we have
enough of a universal pattern yet to document something like an
“in-progress” button in the style guide, since this is the only instance
so far.
- I was debating how we pass the logs text into this component:
currently, the `initialize()` method takes as supplier callback; but of
course, we could also pass in the logs text as string copy directly. I
don’t feel super strongly here, but it felt better to me to not make a
hard copy at initialization time, but to rather “freshly request” the
logs text when the user actually presses the button. The effect is the
same for both approaches, so this is mostly a stylistic issue, and maybe
also a question of data ownership.
- Currently, when `logs.tinypilotkvm.com` would fail for whatever
reason, the button would just bounce back to its initial state. If we
feel that we should give clearer feedback about this scenario, we could
consider e.g. toggling the button label to “Error!” for 2 seconds. (That
would have some extra complexity, though, so I thought it might be fine
as is, assuming that the failure is not too likely.)
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1335"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
jotaen4tinypilot added a commit that referenced this issue Mar 28, 2023
Part of #1307.

This PR includes the full text of the update logs in the `details` field
of the `DialogFailedEvent`, i.e. [whatever we previously appended and
displayed in the ongoing update
logs](https://github.com/tiny-pilot/tinypilot/blob/8f9b03b5d7e2712a0ade6d8e6966ccad58699841/app/templates/custom-elements/update-dialog.html#L305-L310).

In a later (but independent) PR, we’ll add the `<share-logs-button>` to
the `<error-dialog>` after update failures, so that the text can be
shared more easily.

Note, this is the first instance were the `details` text is
intentionally lengthy; usually, it’s one or two sentences. While seeing
now how long texts actually render, I’d suggest to reduce the
`max-height` of the “Details” box a bit, to avoid the “Close” button of
the dialog being pushed down too much – especially on smaller view
ports.

For testing this, I found it handy to make the [`getUpdateStatus`
controller](https://github.com/tiny-pilot/tinypilot/blob/8f9b03b5d7e2712a0ade6d8e6966ccad58699841/app/static/js/controllers.js#L160)
return a hard-coded error (to provoke and trigger the failure in the
frontend).

<img width="1057" alt="Screenshot 2023-03-24 at 16 12 05"
src="https://user-images.githubusercontent.com/83721279/227566040-8b7b32a7-9bb0-4cc2-8c7b-fc5fb9b0b5ea.png">

<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1338"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
jotaen4tinypilot added a commit that referenced this issue Mar 28, 2023
Part of #1307, stacked on
#1335.

This PR replaces the custom logs-sharing functionality in the
`<debug-dialog>` with the new general purpose `<share-logs-button>`.


https://user-images.githubusercontent.com/83721279/227215669-c32b737f-5e02-486a-a98c-5be00c50e8b0.mov

One note about “hiding sensitive data”: after you requested a shareable
URL, if you press the “hide sensitive data” toggle, then the share
button resets to its initial state. I think you can make good points
either way: on the one hand, after you change the appearance of the logs
text, the previously uploaded logs text is “stale”. On the other hand,
it might be clear anyway that the uploaded logs text is out of your
control after you have uploaded it. Apart from usability aspects, the
current solution is a bit easier to build, though, and I’m also thinking
that uploading logs is a cheap and simple operation, so having to
re-trigger it might also not be *that* bad.
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1336"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
jotaen4tinypilot added a commit that referenced this issue Mar 28, 2023
Part of #1307.

As discussed in the ticket, we want to offer the option to share error
logs via a shareable URL, but only for certain errors which we know to
be lengthy. We can control this via a new, optional parameter
`isShareable` in the `DialogFailedEvent`.

The usage is demonstrated in
#1340.

I’ve done a few minor layout adjustments in the `<error-dialog>`, to
account for the different spacing requirements due to the new share
button.

I also noticed that we could pull out the inlined
`this.shadowRoot.getElementById` invocations to a `this.elements` field
(as we usually do), and I’m happy to do that as subsequent refactoring.
(Note to self: don’t forget this.)
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1339"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
@jdeanwallace
Copy link
Contributor

@jotaen4tinypilot - I think this issue is done and can be closed right?

@jotaen4tinypilot
Copy link
Contributor

Yes, thanks for the nudge! (I accidentally wrote Resolves of #1307 instead of Resolves #1307 in the final PR 🤦 , so it didn’t close.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants