status-indicator: Display external service syncing errors #5319
Conversation
mrnugget
force-pushed
the
core/ext-svc-errors-2
branch
4 times, most recently
from
9f91fe2
to
48b7daa
Compare
Codecov Report
@@ Coverage Diff @@
## master #5319 +/- ##
==========================================
+ Coverage 45.78% 45.87% +0.09%
==========================================
Files 736 736
Lines 45416 45523 +107
Branches 2621 2625 +4
==========================================
+ Hits 20793 20885 +92
- Misses 22612 22624 +12
- Partials 2011 2014 +3
|
Codecov Report
@@ Coverage Diff @@
## master #5319 +/- ##
==========================================
+ Coverage 46.07% 46.13% +0.06%
==========================================
Files 735 735
Lines 45176 45263 +87
Branches 2621 2626 +5
==========================================
+ Hits 20813 20882 +69
- Misses 22349 22363 +14
- Partials 2014 2018 +4
|
|
Thanks for the great suggestions, @lguychard! Wil incorporate them today!
I don't think they should be mutually exclusive. There's multiple cases where it's useful to have this:
Point being: external services can be totally independent of each other and I think it makes sense to show the cloning message even if one external service produces errors. It's also an added signal: does the number of to-be-cloned repos change while the error is displayed? Or is it not? Could help debugging |
mrnugget
force-pushed
the
core/ext-svc-errors-2
branch
from
a9411a0
to
7aee050
Compare
I understand this need, but this isn't conveyed clearly (to me) in the current implementation, mostly because the cloning message, as shown, is not specific to an external service, while the error messages are. This makes it unclear to me whether the errors are blocking the enqueued repositories from being cloned, or whether cloning of some repositories is still happening in spite of the error. |
I see. Good point. Solving this is not quite that easy, I feel, because of the second part:
The errors could block the cloning, but not necessarily. Off the top of my head, there's two potential solutions to this:
|
cmd/repo-updater/repos/syncer.go
Outdated
| // produces an error through multiple syncs, the state of | ||
| // multiSourceErr would flip-flop between here and the `ListRepos` call | ||
| // further down. | ||
| s.SetOrResetMultiSourceErr(err) |
There was a problem hiding this comment.
github won't let me comment on it, but what if ListExternalServices fails? Maybe we should just do this at the layer we called sourced?
There was a problem hiding this comment.
ListExternalServices won't return a SourceError/MultiSourceError, which means it doesn't come with an external service attached to it.
I agree, though, that we should display ListExternalServices errors and also UpsertRepos errors, for that matter, since the latter was involved in a a few customer issues in the past (unique constraint errors, etc.).
That would mean that we generalize and go from a multiSourceErr *MultiSourceError field on Syncer to a lastSyncErr error field (which is what I had in my first internal proof-of-concept version of this). On the caller side, in Server, we could then differentiate between a normal error (without external service!) and a MultiSourceError (with external service!) and produce two different status messages — SyncErrorStatusMessage and ListExternalServiceErrorStatusMessage.
What do you think? Should that be part of this PR?
Edit: the more I think about that, the more it makes sense to me:
- change the syncing error status messages to have an optional (!) external service id field
- turn the error field on
Syncerinto aerror - whenever a call in
Sync(notsourced!) produces an error (including the DB functions), set this error, abort sync - depending on whether or not the
lastSyncErroris aMultiSourceErrorwithSourcesor not, attach an external service id to the status message
Pending any objections, I'll hack on this
There was a problem hiding this comment.
agreed this sounds good to me!
|
On the styling, the dividers that don't extend to the edges of the popover look odd to me. Also, why dividers if the alerts are each in a box? Are they trying to convey some semantic groups? If yes, it is no obvious... |
Agree. I was playing around with a few different styles but none looked really good to me. Let me try if I can whip some options together. |
mrnugget
force-pushed
the
core/ext-svc-errors-2
branch
from
10fe055
to
78a8cdb
Compare
|
As long as this targets 3.8 and we're gonna polish it before the release, I'm fine with merging and following up on the design |
|
Just pushed some styling tweaks:
When everything is cloned:
With a
I'm happy with this, but @christinaforney has some ideas, so: feel free to play with this branch and let me know whether I can merge it tomorrow or not :) |
|
Here is what I came up with for the styles. Let me know what you think! I was hacking the DOM a bit, so anywhere you see an emoji, please use a similar icon there instead. I would like to see the icon indicating type of the notice: The icon should lead the heading for proximity association, and visual representation. I used a Here is a warning in light/dark: Success state in light/dark: In progress and error states in light/dark: |
|
I've implemented @christinaforney's ideas. Light theme
Dark theme
We've decided that we can still iterate on this in this iteration. If someone with more design chops comes up with something better: please speak up 😄 |
|
Looks great! 😁 |
|
I don't think the "line on the side" style is a bad one, but it feels a bit inconsistent to me with how we represent alerts elsewhere. I am also missing a bit of visual separation between the items, which in this design, I would solve by using the same color slighty-transparent as a background for each item. But then it really would be just an alert design, but one that is entirely different from other styles. I usually see the "line on the side" style used in design systems that prefer sharp edges & corners over rounded ones. But our design uses rounded edges everywhere. You could make the line a @christinaforney is there a reason why you prefer this design over our existing alert styles? For example, if it is the "box in a box" that is bothering you, maybe we could try out extending the alerts to the edges. 👍 on icons though |
|
Really good questions! I don't think these should be considered alerts, instead they are notifications or a list of messages. Because of this I think they need a different style than the alerts. My issue with them was around readability of so many stacked colors, which is what got me to realize that they aren't actually alerts in the context of the drop down. I agree about the rounded edges and don't have a good alternative - the design consistency with the rest of the product was the thing I was most worried about with the design I suggested. Do you have any ideas to find a middle ground? What do you think of adding back the hr separation between items with the current design? |
|
What do you think of my idea to extend the background to the sides? E.g. https://getbootstrap.com/docs/4.3/components/list-group/#contextual-classes |
This is a follow-up to #5319 and implements the improved error reporting as described in this comment: https://github.com/sourcegraph/sourcegraph/pull/5319#discussion_r316627988 The user visible changes: * The types of `StatusMessage`s have been changed so that there is now an `ExternalServiceSyncError` to which an external service is attached and the new `SyncError` without an external service * The `SyncError` will now be produced whenever the syncing process in repo-updater fails, even if that doesn't involve an external service. That means we now, for example, display errors that originate in the database. That's something customer's ran into in the past but that was never visible.
This is a follow-up to #5319 and implements the improved error reporting as described in this comment: https://github.com/sourcegraph/sourcegraph/pull/5319#discussion_r316627988 The user visible changes: * The types of `StatusMessage`s have been changed so that there is now an `ExternalServiceSyncError` to which an external service is attached and the new `SyncError` without an external service * The `SyncError` will now be produced whenever the syncing process in repo-updater fails, even if that doesn't involve an external service. That means we now, for example, display errors that originate in the database. That's something customer's ran into in the past but that was never visible.
* status-indicator: Display all syncing errors from repo-updater This is a follow-up to #5319 and implements the improved error reporting as described in this comment: https://github.com/sourcegraph/sourcegraph/pull/5319#discussion_r316627988 The user visible changes: * The types of `StatusMessage`s have been changed so that there is now an `ExternalServiceSyncError` to which an external service is attached and the new `SyncError` without an external service * The `SyncError` will now be produced whenever the syncing process in repo-updater fails, even if that doesn't involve an external service. That means we now, for example, display errors that originate in the database. That's something customer's ran into in the past but that was never visible. * fixup! Make setOrResetLastSyncErr easier to understand * fixup! graphql: Prefix statusmessage with `FOR INTERNAL USE`
I don't like all the text being in the colorful background. What do you think of this alternative (imagine the title colors going all the way to the edges). I think it adds a nice visual separation from the content and title and would layer nicely.
|
This is the revised version of the (never merged) PR #4780 and the first attempt at fixing #5146.
TL;DR: This is step 1/n. It shows
repo-updatererrors in the UI. They look raw, but I think that's fine, because we finally show errors and I'm now also happy with the underlying implementation and think that we can iterate on the details (see later sections)What this PR does it to make the errors visible that
repo-updaterencounters when syncing repositories from external services. These errors can be produced when trying to construct an external service from the existing configuration or when talking to the external service to list the available repositories for sync.These errors then get shown in the UI, like this:
Implementation
The implementation in this PR is based on #4780:
SyncErrorStatusMessagerepo-updaterin its/status-messagesendpoint when a sync error has been produced in the last sync runIn addition to that it also contains the suggestions made by @felixfbecker and @tsenart:
StatusMessagetype in the GraphQL schema has been turned into a union type. I like this approach a lot more, because it not only gives us type-safety in TypeScript, but (at the cost of verbosity) also gives us better type-safety ingraphqlbackendandrepo-updater.StatusMessageinrepoupdater/protocola bit unwiedly. But that's one of the possible I discussed with @tsenart and I like it a lot more than the untyped list of key-value pairs.alert-dangerwhich makes them pop out a lot more as errorsOpen issues, drawbacks & thoughts for enhancements
Idea: I prototyped to define a set of errors that is common across external services (bad credentials, connection error, rate limit, ...) and then wrap the errors produced by each external service into one of these custom errors. In the
repo-updaterendpoint we could then detect these errors and replace their messages with something meaningful. The drawback of that is that it's hard to define this common set so it makes sense for all external services and wrapping each the errors produced by each one requires knowledge of each external service's idosyncracies. That's a lot of work, but we could approach it step by step and wrap/beautify these errors as they arise.repo-updaterproduces. Example of such a warning is "repository not found" which is produced when we try to fetch the configured repositories but can't find one of them. This warning is only written to the log and never returned as an error, because we want togitserverruns into when cloning/fetching/cleaning repositories. Until now I haven't looked into makinggitservererrors visible, but that's certainly a large part of what would make fixing Make external service syncing errors visible to site-admins #5146 valuableEspecially 2 and 3 make me question whether the current "architecture" will hold up: if we only care about errors, it's easy to remember the last error and reset it when everything's fine again. But what do we do when we want to display ephemeral data, such as warnings? Where and how do we collect it? (Theoretically we could change our function's return signature from
<val>, errorto<val>, warningstring, error, but that's ... ugly). I keep thinking that we need to introduce something (anErrorReporter?) that other functions/methods could call to report an issue. That would be far easier than wrapping, passing and unwrapping errors along the call chain in the syncer, with the possible added cost of introducing a singleton. And, still: what do we do with warnings?Happy about any input/feedback/ideas!
Test Plan
Test plan: manual testing, go test, cd web && yarn test --runInBand StatusMessagesNavItem.test.tsx