Skip to content

Conversation

bmscomp
Copy link
Contributor

@bmscomp bmscomp commented Mar 1, 2020

Actually heath indicator details for Neo4j returns the number of nodes witch makes one database hit

  neo4j: {
  status: "UP",
  details: {
      nodes: 0
    }
  }

Comparing to other database vendors, I found more useful to show technical details (version and edition) about the database rather then returning the number of nodes in the database

With this pull request the Neo4j heath details will be as follow

  neo4j: {
  status: "UP",
  details: {
      edition: "community",
      version: "4.0.0"
    }
  }

Notice that the new request will make 0 hits to the database, and I guess less time consuming then counting the total number of nodes

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 1, 2020
@snicoll
Copy link
Member

snicoll commented Mar 1, 2020

Thanks for the PR

Notice that the new request will make 0 hits to the database,

I think that can be a problem as making a hit to the database is validating the connection is operating properly. Not doing so may lead to an UP indicator while the access to the database (or the database itself) is down.

paging @michael-simons for insights.

@michael-simons
Copy link
Contributor

That’s a valuable change. I actually don’t like the original query at all as it goes to the node store.
The query here goes over the wire to the database, so the connection is verified. No issues on that side.

I’m unsure however since when Neo4j has that stored procedure and OGM still supports 3.2. Please make also sure it works on embedded.

For the alternate starter that uses bolt only and doesn’t bring in OGM, I’ll probably add the same information. Like it a lot.

For comparison here’s what we do over there
https://github.com/neo4j/neo4j-java-driver-spring-boot-starter/blob/master/neo4j-java-driver-spring-boot-autoconfigure/src/main/java/org/neo4j/driver/springframework/boot/actuate/Neo4jHealthIndicator.java#L99

@bmscomp
Copy link
Contributor Author

bmscomp commented Mar 2, 2020

Yes the updated CYPHER query works for version 3.2 and on embedded version of neo4j and of course 4.0.0 , is there any need to add an integration test for those versions

@michael-simons
Copy link
Contributor

michael-simons commented Mar 2, 2020

@snicoll Big thumbs up from my side.

Much less impact on the database while still verifying the connection and better info.

@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 4, 2020
@snicoll snicoll changed the title Update details for Neo4j health indicator (version and edition) Include version and edition of neo4j database in health details Mar 4, 2020
@snicoll snicoll self-assigned this Mar 4, 2020
@snicoll snicoll added this to the 2.3.0.M3 milestone Mar 4, 2020
snicoll pushed a commit that referenced this pull request Mar 4, 2020
This commit changes the neo4j health indicator to provide the version
and edition of the neo4j database.

See gh-20356
snicoll added a commit that referenced this pull request Mar 4, 2020
@snicoll snicoll closed this in 41101f7 Mar 4, 2020
@snicoll
Copy link
Member

snicoll commented Mar 4, 2020

@bmscomp thanks a lot for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants