Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

status-indicator: Display external service validation and syncing errors #4804

Closed
wants to merge 12 commits into from
13 changes: 12 additions & 1 deletion cmd/frontend/graphqlbackend/schema.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 12 additions & 1 deletion cmd/frontend/graphqlbackend/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -3692,9 +3692,18 @@ type ProductSubscriptionEvent {
url: String
}

# The type of a StatusMessage
# The type of a StatusMessage.
enum StatusMessageType {
CLONING
SYNCERROR
}

# A single metadata element attached to a statusmessage.
type StatusMessageMetadata {
# The name of this metadata pair.
name: String!
# The data of this metadata pair.
value: String!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is this type just a way to get an untyped key/value bag? Why can't this metadata be typed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the metadata differs depending on which type the StatusMessage has. AFAIK GraphQL doesn't support union types, which is what I'd want here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GraphQL does support union types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, interesting! 🤔

}

# A status message
Expand All @@ -3703,4 +3712,6 @@ type StatusMessage {
message: String!
# The type.
type: StatusMessageType!
# List of metadata
metadata: [StatusMessageMetadata!]!
}
47 changes: 45 additions & 2 deletions cmd/frontend/graphqlbackend/status_messages.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@ package graphqlbackend

import (
"context"
"fmt"
"strconv"

"github.com/sourcegraph/sourcegraph/cmd/frontend/backend"
"github.com/sourcegraph/sourcegraph/cmd/frontend/types"
"github.com/sourcegraph/sourcegraph/pkg/repoupdater"
"github.com/sourcegraph/sourcegraph/pkg/repoupdater/protocol"
)

func (r *schemaResolver) StatusMessages(ctx context.Context) ([]*statusMessageResolver, error) {
Expand All @@ -22,9 +25,17 @@ func (r *schemaResolver) StatusMessages(ctx context.Context) ([]*statusMessageRe
}

for _, rn := range result.Messages {
fmt.Printf("rn=%+v\n", rn)
metadata := make([]types.StatusMessageMetadata, len(rn.Metadata))

for i, m := range rn.Metadata {
metadata[i] = types.StatusMessageMetadata{Name: m.Name, Value: m.Value}
}

messages = append(messages, &statusMessageResolver{&types.StatusMessage{
Message: rn.Message,
Type: string(rn.Type),
Message: rn.Message,
Type: string(rn.Type),
Metadata: metadata,
}})
}

Expand All @@ -37,3 +48,35 @@ type statusMessageResolver struct {

func (n *statusMessageResolver) Type() string { return n.message.Type }
func (n *statusMessageResolver) Message() string { return n.message.Message }
func (n *statusMessageResolver) Metadata() []*statusMessageMetadataResolver {
var resolvers []*statusMessageMetadataResolver

for _, m := range n.message.Metadata {
resolvers = append(resolvers, &statusMessageMetadataResolver{
messageType: protocol.StatusMessageType(n.message.Type),
metadata: m,
})
}

return resolvers
}

type statusMessageMetadataResolver struct {
messageType protocol.StatusMessageType
metadata types.StatusMessageMetadata
}

func (n *statusMessageMetadataResolver) Name() string { return n.metadata.Name }
func (n *statusMessageMetadataResolver) Value() (string, error) {
if n.messageType == protocol.SyncingErrorMessage && n.Name() == "ext_svc_id" {
mrnugget marked this conversation as resolved.
Show resolved Hide resolved
id, err := strconv.ParseInt(n.metadata.Value, 10, 64)
if err != nil {
return "", err
}

graphqlID := marshalExternalServiceID(id)
return string(graphqlID), nil
}

return n.metadata.Value, nil
}
10 changes: 8 additions & 2 deletions cmd/frontend/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,13 @@ type SurveyResponse struct {
CreatedAt time.Time
}

type StatusMessageMetadata struct {
Name string
Value string
}

type StatusMessage struct {
Message string
Type string
Message string
Type string
Metadata []StatusMessageMetadata
}
72 changes: 65 additions & 7 deletions cmd/repo-updater/repos/sources.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type Sourcer func(...*ExternalService) (Sources, error)
func NewSourcer(cf *httpcli.Factory, decs ...func(Source) Source) Sourcer {
return func(svcs ...*ExternalService) (Sources, error) {
srcs := make([]Source, 0, len(svcs))
errs := new(multierror.Error)
errs := new(MultiSourceError)

for _, svc := range svcs {
if svc.IsDeleted() {
Expand All @@ -35,7 +35,7 @@ func NewSourcer(cf *httpcli.Factory, decs ...func(Source) Source) Sourcer {

src, err := NewSource(svc, cf)
if err != nil {
errs = multierror.Append(errs, err)
errs.Append(&SourceError{Err: err, ExtSvc: svc})
continue
}

Expand Down Expand Up @@ -84,6 +84,60 @@ type Source interface {
ExternalServices() ExternalServices
}

type SourceError struct {
Err error
ExtSvc *ExternalService
}

func (s *SourceError) Error() string {
if multiErr, ok := s.Err.(*multierror.Error); ok {
multiErr.ErrorFormat = sourceErrorFormatFunc
return multiErr.Error()
}
return s.Err.Error()
}

func sourceErrorFormatFunc(es []error) string {
if len(es) == 1 {
return es[0].Error()
}

points := make([]string, len(es))
mrnugget marked this conversation as resolved.
Show resolved Hide resolved
for i, err := range es {
points[i] = fmt.Sprintf("* %s", err)
}

return fmt.Sprintf(
"%d errors occurred:\n\t%s\n\n",
len(es), strings.Join(points, "\n\t"))
}

type MultiSourceError struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes this type necessary? Can't we work with multierror.Error of SourceErrors?

Copy link
Contributor Author

@mrnugget mrnugget Jul 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I did first, but it lead to the code looking like this:

if multiErr, ok := err.(*multierror.Error); ok {
	for _, sub := range multiErr.Errors {
		if sourceErr, ok := sub.(*SourceError); ok {
			m := newSyncErrMessage(sourceErr)
			resp.Messages = append(resp.Messages, m)
		} else {
			// drop error? add generic message?
		}
	}
} else {
	// drop error? add generic message?
}

That seems a bit unwieldy and MultiSourceError was me trying my hand at putting some more information back into the type system. But I'm not set on it.

I think the general problem that needs to be solved is the association of errors to external services and making those accessible to consumers. multierror.Error and MultiSourceError are equally capable of doing this, but I feel like there might be an even better approach.

I'm also thinking that maybe we need a map that gets updated with each sync run, kinda like map[*ExternalService]error. That would then also allow us to show the status per external service when everything went fine. But in order to implement that, we still need to find out what happened with each external service inside the syncer, so at least *SourceError seems necessary?

And another problem is that, of course, the Err in *SourceError is also a multierror.Error and doesn't lead to the most UI-friendly messages (2 errors occured:\n\t* blub\n\t* blah...). I thought about using a custom format func for this, but that would only lead to a custom format in repo-updater that then needs to be untangled in the TypeScript world?

Errors []*SourceError
}

func (s *MultiSourceError) Error() string {
errs := new(multierror.Error)
for _, e := range s.Errors {
errs = multierror.Append(errs, e)
}
return errs.Error()
}

func (s *MultiSourceError) Append(errs ...*SourceError) {
s.Errors = append(s.Errors, errs...)
}

func (s *MultiSourceError) ErrorOrNil() error {
if s == nil {
return nil
}
if len(s.Errors) == 0 {
return nil
}
return s
}

// Sources is a list of Sources that implements the Source interface.
type Sources []Source

Expand All @@ -97,7 +151,7 @@ func (srcs Sources) ListRepos(ctx context.Context) ([]*Repo, error) {
type result struct {
src Source
repos []*Repo
err error
errs []*SourceError
}

// Group sources by external service kind so that we execute requests
Expand All @@ -113,7 +167,11 @@ func (srcs Sources) ListRepos(ctx context.Context) ([]*Repo, error) {
defer wg.Done()
for _, src := range sources {
if repos, err := src.ListRepos(ctx); err != nil {
ch <- result{src: src, err: err}
var errs []*SourceError
for _, extSvc := range src.ExternalServices() {
errs = append(errs, &SourceError{Err: err, ExtSvc: extSvc})
}
ch <- result{src: src, errs: errs}
} else {
ch <- result{src: src, repos: repos}
}
Expand All @@ -127,11 +185,11 @@ func (srcs Sources) ListRepos(ctx context.Context) ([]*Repo, error) {
}()

var repos []*Repo
errs := new(multierror.Error)
errs := new(MultiSourceError)

for r := range ch {
if r.err != nil {
errs = multierror.Append(errs, r.err)
if len(r.errs) != 0 {
errs.Append(r.errs...)
} else {
repos = append(repos, r.repos...)
}
Expand Down
31 changes: 30 additions & 1 deletion cmd/repo-updater/repos/syncer.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"sort"
"strconv"
"strings"
"sync"
"time"

otlog "github.com/opentracing/opentracing-go/log"
Expand All @@ -22,6 +23,12 @@ type Syncer struct {
// Sourcegraph.com
FailFullSync bool

// LastSourcerError contains the last error returned by the Sourcer in each
// Sync. It's reset with each Sync and if the Sourcer produced no error,
// it's set to nil.
mrnugget marked this conversation as resolved.
Show resolved Hide resolved
multiSourceErr *MultiSourceError
multiSourceErrMu sync.RWMutex

store Store
sourcer Sourcer
diffs chan Diff
Expand Down Expand Up @@ -310,14 +317,36 @@ func (s *Syncer) sourced(ctx context.Context) ([]*Repo, error) {
}

srcs, err := s.sourcer(svcs...)
s.SetOrResetMultiSourceErr(err)
if err != nil {
return nil, err
}

ctx, cancel := context.WithTimeout(ctx, sourceTimeout)
defer cancel()

return srcs.ListRepos(ctx)
repos, err := srcs.ListRepos(ctx)
s.SetOrResetMultiSourceErr(err)

return repos, err
}

func (s *Syncer) SetOrResetMultiSourceErr(err error) {
s.multiSourceErrMu.Lock()

if multiSourceErr, ok := err.(*MultiSourceError); ok {
s.multiSourceErr = multiSourceErr
} else {
s.multiSourceErr = nil
}

s.multiSourceErrMu.Unlock()
}

func (s *Syncer) MultiSourceError() *MultiSourceError {
s.multiSourceErrMu.RLock()
defer s.multiSourceErrMu.RUnlock()
return s.multiSourceErr
}

func (s *Syncer) observe(ctx context.Context, family, title string) (context.Context, func(*Diff, *error)) {
Expand Down
30 changes: 20 additions & 10 deletions cmd/repo-updater/repos/syncer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,25 +29,28 @@ func TestSyncer_Sync(t *testing.T) {
gitlab := repos.ExternalService{ID: 2, Kind: "gitlab"}

for _, tc := range []struct {
name string
sourcer repos.Sourcer
store repos.Store
err string
name string
sourcer repos.Sourcer
store repos.Store
err string
lastSourcerErr string
}{
{
name: "sourcer error aborts sync",
sourcer: repos.NewFakeSourcer(errors.New("boom")),
store: new(repos.FakeStore),
err: "syncer.sync.sourced: boom",
name: "sourcer error aborts sync",
sourcer: repos.NewFakeSourcer(errors.New("boom")),
store: new(repos.FakeStore),
err: "syncer.sync.sourced: 1 error occurred:\n\t* boom\n\n",
lastSourcerErr: "1 error occurred:\n\t* boom\n\n",
},
{
name: "sources partial errors aborts sync",
sourcer: repos.NewFakeSourcer(nil,
repos.NewFakeSource(&github, nil),
repos.NewFakeSource(&gitlab, errors.New("boom")),
),
store: new(repos.FakeStore),
err: "syncer.sync.sourced: 1 error occurred:\n\t* boom\n\n",
store: new(repos.FakeStore),
err: "syncer.sync.sourced: 1 error occurred:\n\t* boom\n\n",
lastSourcerErr: "1 error occurred:\n\t* boom\n\n",
},
{
name: "store list error aborts sync",
Expand All @@ -74,6 +77,13 @@ func TestSyncer_Sync(t *testing.T) {
if have, want := fmt.Sprint(err), tc.err; have != want {
t.Errorf("have error %q, want %q", have, want)
}

if tc.lastSourcerErr == "" {
tc.lastSourcerErr = "<nil>"
}
if have, want := fmt.Sprint(syncer.MultiSourceError()), tc.lastSourcerErr; have != want {
t.Errorf("have error %q, want %q", have, want)
}
})
}
}
Expand Down
15 changes: 13 additions & 2 deletions cmd/repo-updater/repos/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,19 @@ import (
// NewFakeSourcer returns a Sourcer which always returns the given error and sources,
// ignoring the given external services.
func NewFakeSourcer(err error, srcs ...Source) Sourcer {
return func(...*ExternalService) (Sources, error) {
return srcs, err
return func(svcs ...*ExternalService) (Sources, error) {
errs := new(MultiSourceError)

if err != nil {
for _, svc := range svcs {
errs.Append(&SourceError{Err: err, ExtSvc: svc})
}
if len(svcs) == 0 {
errs.Append(&SourceError{Err: err, ExtSvc: nil})
}
}

return srcs, errs.ErrorOrNil()
}
}

Expand Down
Loading