From cb491564b344c14e8b8f96c3a648c96a7201c27d Mon Sep 17 00:00:00 2001 From: Nitin Kumar Date: Thu, 24 Sep 2026 16:00:08 +0530 Subject: [PATCH] fix(arangodb): register metrics and return not-connected errors when Connect fails Connect registered the app_arango_stats histogram only after the first Version call succeeded, so if ArangoDB was down at startup the metric was missing for the whole process. Register it at the start of Connect. When config validation failed, Connect left a nil driver client and HealthCheck plus every DB/collection/document/graph/user method panicked. Route driver access through accessors that return errNotConnected, and report DOWN with details.error in both HealthCheck failure branches. Fixes #4369 --- docs/datasources/arangodb/page.md | 2 + pkg/gofr/datasource/arangodb/arango.go | 37 +++- pkg/gofr/datasource/arangodb/arango_db.go | 20 ++- pkg/gofr/datasource/arangodb/arango_graph.go | 6 +- pkg/gofr/datasource/arangodb/arango_helper.go | 32 +++- pkg/gofr/datasource/arangodb/arango_test.go | 163 ++++++++++++++---- 6 files changed, 201 insertions(+), 59 deletions(-) diff --git a/docs/datasources/arangodb/page.md b/docs/datasources/arangodb/page.md index 68982462f0..1865611626 100644 --- a/docs/datasources/arangodb/page.md +++ b/docs/datasources/arangodb/page.md @@ -18,6 +18,8 @@ To connect to `ArangoDB`, you need to provide the following environment variable - `PASSWORD`: The password for the specified user. - `PORT`: The port number +If any of these values is missing, GoFr logs the error, the ArangoDB health check reports `DOWN` with the reason in `details.error`, and ArangoDB calls return a "not connected to ArangoDB" error instead of panicking. If the server is unreachable at startup, GoFr logs the error and keeps the client; the health check reports `DOWN` (with the connection error in `details.error`) until the server becomes reachable, after which queries and the `app_arango_stats` metric work without a restart. + ## Setup GoFr supports injecting `ArangoDB` that implements the following interface. Any driver that implements the interface can be diff --git a/pkg/gofr/datasource/arangodb/arango.go b/pkg/gofr/datasource/arangodb/arango.go index bdb4b5e65f..2189562332 100644 --- a/pkg/gofr/datasource/arangodb/arango.go +++ b/pkg/gofr/datasource/arangodb/arango.go @@ -17,6 +17,7 @@ import ( const ( defaultTimeout = 5 * time.Second arangoEdgeCollectionType = 3 + statusDown = "DOWN" ) // Client represents an ArangoDB client. @@ -56,6 +57,7 @@ var ( ErrDatabaseExists = errors.New("database already exists") ErrCollectionExists = errors.New("collection already exists") ErrGraphExists = errors.New("graph already exists") + errNotConnected = errors.New("not connected to ArangoDB, check the Connect error logs") ) // New creates a new ArangoDB client with the provided configuration. @@ -94,6 +96,14 @@ func (c *Client) UseTracer(tracer any) { // Connect establishes a connection to the ArangoDB server. func (c *Client) Connect() { + // Register the histogram before any step that can fail, so operations record latency even when + // the server is unreachable at startup and becomes available later. + arangoBuckets := []float64{ + 50, 75, 100, 125, 150, 200, 300, 500, 750, 1000, 2000, 3000, 5000, 7500, 10000, // 50µs-10ms + 25000, 50000, 100000, 250000, 500000, 1000000, 5000000, 10000000, 30000000, 60000000, 120000000, 180000000, // 25ms-3min + } + c.metrics.NewHistogram("app_arango_stats", "Response time of ArangoDB operations in microseconds.", arangoBuckets...) + if err := c.validateConfig(); err != nil { c.logger.Errorf("config validation error: %v", err) return @@ -127,14 +137,16 @@ func (c *Client) Connect() { return } - // Initialize metrics - arangoBuckets := []float64{ - 50, 75, 100, 125, 150, 200, 300, 500, 750, 1000, 2000, 3000, 5000, 7500, 10000, // 50µs-10ms - 25000, 50000, 100000, 250000, 500000, 1000000, 5000000, 10000000, 30000000, 60000000, 120000000, 180000000, // 25ms-3min + c.logger.Logf("Connected to ArangoDB successfully at %s", c.endpoint) +} + +// arangoClient returns the underlying driver client, or errNotConnected when Connect failed before creating it. +func (c *Client) arangoClient() (arangodb.Client, error) { + if c.client == nil { + return nil, errNotConnected } - c.metrics.NewHistogram("app_arango_stats", "Response time of ArangoDB operations in microseconds.", arangoBuckets...) - c.logger.Logf("Connected to ArangoDB successfully at %s", c.endpoint) + return c.client, nil } func (c *Client) validateConfig() error { @@ -239,7 +251,7 @@ func (c *Client) Query(ctx context.Context, dbName, query string, bindVars map[s ctx, done := c.instrumentOp(ctx, &QueryLog{Operation: "query", Database: dbName, Query: query}) defer done() - db, err := c.client.GetDatabase(ctx, dbName, nil) + db, err := c.database(ctx, dbName) if err != nil { return err } @@ -363,9 +375,18 @@ func (c *Client) HealthCheck(ctx context.Context) (any, error) { }, } + if c.client == nil { + h.Status = statusDown + h.Details["error"] = errNotConnected.Error() + + return &h, errNotConnected + } + version, err := c.client.Version(ctx) if err != nil { - h.Status = "DOWN" + h.Status = statusDown + h.Details["error"] = err.Error() + return &h, errStatusDown } diff --git a/pkg/gofr/datasource/arangodb/arango_db.go b/pkg/gofr/datasource/arangodb/arango_db.go index c25fd99807..3df2c41602 100644 --- a/pkg/gofr/datasource/arangodb/arango_db.go +++ b/pkg/gofr/datasource/arangodb/arango_db.go @@ -17,8 +17,13 @@ func (d *DB) CreateDB(ctx context.Context, database string) error { ctx, done := d.client.instrumentOp(ctx, &QueryLog{Operation: "createDB", Database: database}) defer done() + client, err := d.client.arangoClient() + if err != nil { + return err + } + // Check if the database already exists - exists, err := d.client.client.DatabaseExists(ctx, database) + exists, err := client.DatabaseExists(ctx, database) if err != nil { return err } @@ -28,7 +33,7 @@ func (d *DB) CreateDB(ctx context.Context, database string) error { return ErrDatabaseExists } - _, err = d.client.client.CreateDatabase(ctx, database, nil) + _, err = client.CreateDatabase(ctx, database, nil) return err } @@ -38,7 +43,12 @@ func (d *DB) DropDB(ctx context.Context, database string) error { ctx, done := d.client.instrumentOp(ctx, &QueryLog{Operation: "dropDB", Database: database}) defer done() - db, err := d.client.client.GetDatabase(ctx, database, &arangodb.GetDatabaseOptions{}) + client, err := d.client.arangoClient() + if err != nil { + return err + } + + db, err := client.GetDatabase(ctx, database, &arangodb.GetDatabaseOptions{}) if err != nil { return err } @@ -59,7 +69,7 @@ func (d *DB) CreateCollection(ctx context.Context, database, collection string, Collection: collection, Filter: isEdge}) defer done() - db, err := d.client.client.GetDatabase(ctx, database, nil) + db, err := d.client.database(ctx, database) if err != nil { return err } @@ -102,7 +112,7 @@ func (d *DB) DropCollection(ctx context.Context, database, collectionName string } func (d *DB) getCollection(ctx context.Context, dbName, collectionName string) (arangodb.Collection, error) { - db, err := d.client.client.GetDatabase(ctx, dbName, nil) + db, err := d.client.database(ctx, dbName) if err != nil { return nil, err } diff --git a/pkg/gofr/datasource/arangodb/arango_graph.go b/pkg/gofr/datasource/arangodb/arango_graph.go index 5839ab5ab1..df9747c2af 100644 --- a/pkg/gofr/datasource/arangodb/arango_graph.go +++ b/pkg/gofr/datasource/arangodb/arango_graph.go @@ -36,7 +36,7 @@ func (g *Graph) CreateGraph(ctx context.Context, database, graph string, edgeDef Database: database, Graph: graph}) defer done() - db, err := g.client.client.GetDatabase(ctx, database, nil) + db, err := g.client.database(ctx, database) if err != nil { return err } @@ -91,7 +91,7 @@ func (g *Graph) DropGraph(ctx context.Context, database, graphName string) error Database: database, Graph: graphName}) defer done() - db, err := g.client.client.GetDatabase(ctx, database, nil) + db, err := g.client.database(ctx, database) if err != nil { return err } @@ -140,7 +140,7 @@ func (c *Client) GetEdges(ctx context.Context, dbName, graphName, edgeCollection }) defer done() - db, err := c.client.GetDatabase(ctx, dbName, nil) + db, err := c.database(ctx, dbName) if err != nil { return err } diff --git a/pkg/gofr/datasource/arangodb/arango_helper.go b/pkg/gofr/datasource/arangodb/arango_helper.go index 6d1e9990e7..bf04bd8da0 100644 --- a/pkg/gofr/datasource/arangodb/arango_helper.go +++ b/pkg/gofr/datasource/arangodb/arango_helper.go @@ -8,11 +8,21 @@ import ( ) func (c *Client) user(ctx context.Context, username string) (arangodb.User, error) { - return c.client.User(ctx, username) + client, err := c.arangoClient() + if err != nil { + return nil, err + } + + return client.User(ctx, username) } func (c *Client) database(ctx context.Context, name string) (arangodb.Database, error) { - return c.client.GetDatabase(ctx, name, nil) + client, err := c.arangoClient() + if err != nil { + return nil, err + } + + return client.GetDatabase(ctx, name, nil) } // createUser creates a new user in ArangoDB. @@ -25,7 +35,12 @@ func (c *Client) createUser(ctx context.Context, username string, options any) e return fmt.Errorf("%w", errInvalidUserOptionsType) } - _, err := c.client.CreateUser(ctx, username, userOptions.toArangoUserOptions()) + client, err := c.arangoClient() + if err != nil { + return err + } + + _, err = client.CreateUser(ctx, username, userOptions.toArangoUserOptions()) if err != nil { return err } @@ -38,7 +53,12 @@ func (c *Client) dropUser(ctx context.Context, username string) error { ctx, done := c.instrumentOp(ctx, &QueryLog{Operation: "dropUser", ID: username}) defer done() - err := c.client.RemoveUser(ctx, username) + client, err := c.arangoClient() + if err != nil { + return err + } + + err = client.RemoveUser(ctx, username) if err != nil { return err } @@ -51,7 +71,7 @@ func (c *Client) grantDB(ctx context.Context, database, username, permission str ctx, done := c.instrumentOp(ctx, &QueryLog{Operation: "grantDB", Database: database, ID: username}) defer done() - user, err := c.client.User(ctx, username) + user, err := c.user(ctx, username) if err != nil { return err } @@ -67,7 +87,7 @@ func (c *Client) grantCollection(ctx context.Context, database, collection, user Database: database, Collection: collection, ID: username}) defer done() - user, err := c.client.User(ctx, username) + user, err := c.user(ctx, username) if err != nil { return err } diff --git a/pkg/gofr/datasource/arangodb/arango_test.go b/pkg/gofr/datasource/arangodb/arango_test.go index 0e6c130479..73692884af 100644 --- a/pkg/gofr/datasource/arangodb/arango_test.go +++ b/pkg/gofr/datasource/arangodb/arango_test.go @@ -20,6 +20,7 @@ var ( errUserNotFound = errors.New("user not found") errDBNotFound = errors.New("database not found") errDocumentNotFound = errors.New("document not found") + errConnRefused = errors.New("connection refused") ) func setupDB(t *testing.T) (*Client, *MockClient, *MockUser, *MockLogger, *MockMetrics) { @@ -47,25 +48,6 @@ func setupDB(t *testing.T) (*Client, *MockClient, *MockUser, *MockLogger, *MockM return client, mockArango, mockUser, mockLogger, mockMetrics } -func Test_NewArangoClient_Error(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - metrics := NewMockMetrics(ctrl) - logger := NewMockLogger(ctrl) - - logger.EXPECT().Errorf("failed to verify connection: %v", gomock.Any()) - logger.EXPECT().Debugf(gomock.Any(), gomock.Any()) - - client := New(Config{Host: "localhost", Port: 8529, Password: "root", User: "admin"}) - - client.UseLogger(logger) - client.UseMetrics(metrics) - client.Connect() - - require.NotNil(t, client) -} - func TestClient_Query_Success(t *testing.T) { test := setupGraphTest(t) defer test.Ctrl.Finish() @@ -194,18 +176,18 @@ func TestClient_HealthCheck_Error(t *testing.T) { test := setupGraphTest(t) defer test.Ctrl.Finish() - test.MockArango.EXPECT().Version(test.Ctx).Return(arangodb.VersionInfo{}, errStatusDown) + test.MockArango.EXPECT().Version(test.Ctx).Return(arangodb.VersionInfo{}, errConnRefused) health, err := test.Client.HealthCheck(test.Ctx) - require.Error(t, err) - require.Equal(t, errStatusDown, err) + require.ErrorIs(t, err, errStatusDown) h, ok := health.(*Health) require.True(t, ok) - require.Equal(t, "DOWN", h.Status) + require.Equal(t, statusDown, h.Status) require.Equal(t, test.Client.endpoint, h.Details["endpoint"]) + require.Equal(t, errConnRefused.Error(), h.Details["error"]) } type MockQueryCursor struct { @@ -445,34 +427,50 @@ func TestClient_Connect(t *testing.T) { })) t.Cleanup(srv.Close) - srvURL, err := url.Parse(srv.URL) - require.NoError(t, err) + downSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + })) + t.Cleanup(downSrv.Close) - port, err := strconv.Atoi(srvURL.Port()) - require.NoError(t, err) + host, port := hostPort(t, srv.URL) + downHost, downPort := hostPort(t, downSrv.URL) tests := []struct { - desc string - config Config - setupMocks func(l *MockLogger, m *MockMetrics) - expEndpoint string + desc string + config Config + setupMocks func(l *MockLogger, m *MockMetrics) + expEndpoint string + expClientSet bool }{ { - desc: "invalid config", + desc: "invalid config registers metrics and leaves client nil", config: Config{Host: "localhost"}, - setupMocks: func(l *MockLogger, _ *MockMetrics) { + setupMocks: func(l *MockLogger, m *MockMetrics) { + m.EXPECT().NewHistogram("app_arango_stats", gomock.Any(), gomock.Any()) l.EXPECT().Errorf("config validation error: %v", gomock.Any()) }, }, + { + desc: "server unreachable still registers metrics", + config: Config{Host: downHost, Port: downPort, User: "root", Password: "root"}, + setupMocks: func(l *MockLogger, m *MockMetrics) { + m.EXPECT().NewHistogram("app_arango_stats", gomock.Any(), gomock.Any()) + l.EXPECT().Debugf("connecting to ArangoDB at %s", "http://"+downSrv.Listener.Addr().String()) + l.EXPECT().Errorf("failed to verify connection: %v", gomock.Any()) + }, + expEndpoint: "http://" + downSrv.Listener.Addr().String(), + expClientSet: true, + }, { desc: "connects and registers metrics", - config: Config{Host: srvURL.Hostname(), Port: port, User: "root", Password: "root"}, + config: Config{Host: host, Port: port, User: "root", Password: "root"}, setupMocks: func(l *MockLogger, m *MockMetrics) { - l.EXPECT().Debugf("connecting to ArangoDB at %s", "http://"+srvURL.Host) m.EXPECT().NewHistogram("app_arango_stats", gomock.Any(), gomock.Any()) - l.EXPECT().Logf("Connected to ArangoDB successfully at %s", "http://"+srvURL.Host) + l.EXPECT().Debugf("connecting to ArangoDB at %s", "http://"+srv.Listener.Addr().String()) + l.EXPECT().Logf("Connected to ArangoDB successfully at %s", "http://"+srv.Listener.Addr().String()) }, - expEndpoint: "http://" + srvURL.Host, + expEndpoint: "http://" + srv.Listener.Addr().String(), + expClientSet: true, }, } @@ -491,6 +489,97 @@ func TestClient_Connect(t *testing.T) { client.Connect() require.Equal(t, tc.expEndpoint, client.endpoint) + require.Equal(t, tc.expClientSet, client.client != nil) }) } } + +func hostPort(t *testing.T, rawURL string) (host string, port int) { + t.Helper() + + u, err := url.Parse(rawURL) + require.NoError(t, err) + + port, err = strconv.Atoi(u.Port()) + require.NoError(t, err) + + return u.Hostname(), port +} + +func TestClient_NotConnected(t *testing.T) { + tests := []struct { + desc string + call func(ctx context.Context, c *Client) error + }{ + {"Query", func(ctx context.Context, c *Client) error { + var res []map[string]any + return c.Query(ctx, "db", "RETURN 1", nil, &res) + }}, + {"CreateDB", func(ctx context.Context, c *Client) error { return c.CreateDB(ctx, "db") }}, + {"DropDB", func(ctx context.Context, c *Client) error { return c.DropDB(ctx, "db") }}, + {"CreateCollection", func(ctx context.Context, c *Client) error { return c.CreateCollection(ctx, "db", "col", false) }}, + {"DropCollection", func(ctx context.Context, c *Client) error { return c.DropCollection(ctx, "db", "col") }}, + {"CreateGraph", func(ctx context.Context, c *Client) error { return c.CreateGraph(ctx, "db", "g", &EdgeDefinition{}) }}, + {"DropGraph", func(ctx context.Context, c *Client) error { return c.DropGraph(ctx, "db", "g") }}, + {"GetEdges", func(ctx context.Context, c *Client) error { + return c.GetEdges(ctx, "db", "g", "edges", "persons/1", &EdgeDetails{}) + }}, + {"CreateDocument", func(ctx context.Context, c *Client) error { + _, err := c.CreateDocument(ctx, "db", "col", map[string]any{}) + return err + }}, + {"GetDocument", func(ctx context.Context, c *Client) error { + return c.GetDocument(ctx, "db", "col", "id", &map[string]any{}) + }}, + {"UpdateDocument", func(ctx context.Context, c *Client) error { + return c.UpdateDocument(ctx, "db", "col", "id", map[string]any{}) + }}, + {"DeleteDocument", func(ctx context.Context, c *Client) error { return c.DeleteDocument(ctx, "db", "col", "id") }}, + {"createUser", func(ctx context.Context, c *Client) error { return c.createUser(ctx, "u", UserOptions{}) }}, + {"dropUser", func(ctx context.Context, c *Client) error { return c.dropUser(ctx, "u") }}, + {"grantDB", func(ctx context.Context, c *Client) error { return c.grantDB(ctx, "db", "u", "rw") }}, + {"grantCollection", func(ctx context.Context, c *Client) error { + return c.grantCollection(ctx, "db", "col", "u", "rw") + }}, + {"user", func(ctx context.Context, c *Client) error { + _, err := c.user(ctx, "u") + return err + }}, + {"database", func(ctx context.Context, c *Client) error { + _, err := c.database(ctx, "db") + return err + }}, + } + + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + ctrl := gomock.NewController(t) + mockLogger := NewMockLogger(ctrl) + mockMetrics := NewMockMetrics(ctrl) + + mockLogger.EXPECT().Debug(gomock.Any()).AnyTimes() + mockMetrics.EXPECT().RecordHistogram(gomock.Any(), "app_arango_stats", gomock.Any(), + gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() + + client := New(Config{}) + client.UseLogger(mockLogger) + client.UseMetrics(mockMetrics) + + err := tc.call(t.Context(), client) + + require.ErrorIs(t, err, errNotConnected) + }) + } +} + +func TestClient_HealthCheck_NotConnected(t *testing.T) { + client := New(Config{}) + + health, err := client.HealthCheck(t.Context()) + + require.ErrorIs(t, err, errNotConnected) + require.Equal(t, &Health{Status: statusDown, Details: map[string]any{ + "endpoint": "", + "error": errNotConnected.Error(), + }}, health) +}