Repository navigation
fix(arangodb): register metrics and return not-connected errors when Connect fails - #4373
Merged
aryanmehrotra merged 2 commits intoOct 13, 2026
Conversation
…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 gofr-dev#4369
This was referenced Sep 24, 2026
aryanmehrotra
approved these changes
Oct 13, 2026
aryanmehrotra
left a comment
Member
There was a problem hiding this comment.
The two fixes are correct. I reviewed commit 7de5957, which is cb49156 merged with development.
Connect() BEFORE AFTER (this PR)
--------- ------ ---------------
register histogram last step, only on success first step, always
config invalid client = nil client = nil
-> DB call panic (nil pointer) returns errNotConnected
-> HealthCheck panic DOWN + details.error
server down at start client kept, no histogram client kept, histogram exists
-> DB call metrics error on every call metric records
-> server comes back works, metric still missing works, metric works
What I ran
go build,go vetandgo test -racepass in thearangodbmodule on this tree, at go-driverv2.4.1.- I removed each fix in turn. The new tests fail each time:
TestClient_NotConnected, 2 rows ofTestClient_Connect, andTestClient_HealthCheck_NotConnected. - I changed one
httptestserver from 503 to 200.HealthCheckreportedDOWN, thenUP, with no secondConnectcall. - I did not use a real ArangoDB server. I did not run lint locally.
Items
- The docs promise recovery after the server becomes reachable. No test covers this. Please add a test that changes one
httptestserver from 503 to 200, and then asserts thatHealthCheckreportsUP. - Nit: the row "server unreachable still registers metrics" uses a server that answers 503. Please rename the row, or use a closed port.
- Question, not a request: a not-connected call still goes through
instrumentOp. It records a sample inapp_arango_statswithendpoint="". Do we want these samples? I read this from the code. I did not measure it. - Nit: the new sentence in
docs/datasources/arangodb/page.mdis more than 40 words long. Please split it into 2 or 3 sentences.
Item 1 is a strong request. Items 2 to 4 are optional.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Connectregistered theapp_arango_statshistogram only after the firstVersioncall succeeded. If ArangoDB was down at startup, the histogram was never created.Connectis not retried, so every later operation logged a metrics error and the latency metric stayed missing for the whole process, even after the server came back.The histogram is now registered at the start of
Connect, as the SurrealDB datasource already does.When config validation or auth setup failed,
Connectreturned with a nil driver client, soHealthCheckand every DB, collection, document, graph and user method panicked with a nil pointer dereference. They now go through a singlearangoClient()/database()/user()accessor that returnserrNotConnected. There are 6 nil guards in total:arangoClient,HealthCheck,CreateDB,DropDB,createUseranddropUser.HealthCheckreportsDOWNwithdetails.errorin both failure cases, so the payload has the same shape either way:Versionfailing, which now includes the underlying error text.The server-down path deliberately keeps its lazily-connecting client, so the datasource recovers once the server becomes reachable.
No public API, interface or mock changes. I checked that it also builds and passes against go-driver v2.4.1, the version #4351 bumps to.
Tests
TestClient_Connectis now table-driven, with rows for invalid config, server unreachable (httptest 503) and success. It replacesTest_NewArangoClient_Error, which depended on nothing listening on localhost:8529.TestClient_NotConnectedhas one row per method that touches the driver.TestClient_HealthCheck_NotConnected.TestClient_HealthCheck_Errornow assertsdetails.error.Docs: the ArangoDB page now describes the failure behaviour.
Possible follow-up (not in this PR):
mongo.Connectregisters its histogram after the ping in the same way.Fixes #4369