Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Micrometer metrics #1137

Merged
merged 28 commits into from
Mar 8, 2022
Merged

Micrometer metrics #1137

merged 28 commits into from
Mar 8, 2022

Conversation

injectives
Copy link
Contributor

No description provided.

shakuzen and others added 4 commits December 15, 2021 01:07
This is a work-in-progress attempt at instrumenting the driver with Micrometer as an optional alternative metrics implementation to the existing internal one.
This update includes general improvements, refactorings and tests.
@injectives injectives marked this pull request as draft January 29, 2022 09:49
@injectives injectives marked this pull request as ready for review January 29, 2022 10:11
@injectives
Copy link
Contributor Author

@shakuzen, big thanks for your effort on this. 👍
We were wondering if you could have a look at this update and let us know if you are happy to have it merged please?
It would be great if you could also sign the CLA agreement.

@shakuzen
Copy link
Contributor

Thank you for the mention and for getting this work in shape for merging. I am checking internally about the CLA and will get back to you.

Copy link
Contributor

@michael-simons michael-simons left a comment

Choose a reason for hiding this comment

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

Can we rethink the optional dependency and go for provided?
Also I'm sceptic whether it will work as expected when Micrometer classes are on the Config class itself depending on the classloader used.

Options I see is using the Java service loader (define or use the existing metrics interface, and try to load it via build Java service loader, find either default or micrometer implementation) or something like I did here https://github.com/neo4j-contrib/cypher-dsl/blob/7c8ba2d4892c2933041fe7343d34c7f0a4b26f43/neo4j-cypher-dsl/src/main/java/org/neo4j/cypherdsl/core/Cypher.java#L1113 (this lets you use QueryDSL with CypherDSL).

I think the most suitable approach here would be service loader, tbh.

driver/src/main/java/org/neo4j/driver/Config.java Outdated Show resolved Hide resolved
- Set scope of micrometer to provided
- Rename MetricsProvider to MetricsAdapter (it does not provide metrics, the driver does. It adapts the drivers metrics to something external)
- Make it a public interface
- Remove dependencies to other internal interfaces and classes (such as BoltServerAddress and the pool itself via ServerAddress and intsuppliers)
- Reduce visibility of internal classes as much as possible
- Make sure that adapters are not serialized
- Apply the clock to the internal adapter (an oversight from the previous iteration)
…r, use global registry by default.

This change keeps the existing serialization feature of config correct.
It will work for everyone just using micrometer as is with the global registry.
if there is a need to select the registriy, we can work on that later.
Copy link
Contributor

@meistermeier meistermeier left a comment

Choose a reason for hiding this comment

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

Do we have to mention somewhere which version(s) of Micrometer is/are supported?
Even though I like the style and would do the same I suggest a refactoring (not part of this PR) away from /dev/null (there is also logging) naming to something different, more understandable for non-neerdy code readers.

driver/src/main/java/org/neo4j/driver/Config.java Outdated Show resolved Hide resolved
driver/src/main/java/org/neo4j/driver/Config.java Outdated Show resolved Hide resolved
…MetricsListener.java

Co-authored-by: Gerrit Meier <meistermeier@gmail.com>
Copy link
Contributor

@meistermeier meistermeier left a comment

Choose a reason for hiding this comment

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

Minor renaming. But if you think that it is better as it is right now, feel free to ignore it.

…terConnectionPoolMetrics.java

Co-authored-by: Gerrit Meier <meistermeier@gmail.com>
Copy link
Contributor

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

This is looking very nice. I see the MicrometerMetricsProvider can take an arbitrary MeterRegistry instance - this is good. How does a user configure things so a specific MeterRegistry is used?

@injectives
Copy link
Contributor Author

This is looking very nice. I see the MicrometerMetricsProvider can take an arbitrary MeterRegistry instance - this is good. How does a user configure things so a specific MeterRegistry is used?

Conceptually, it can take any instance. However, at this iteration and for initial period we are only going to support the global registry.

@michael-simons, please correct me if I am wrong.

Copy link
Contributor

@michael-simons michael-simons left a comment

Choose a reason for hiding this comment

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

Looks good from my site, and yes, @injectives that was the intention: If this feature gets traction we shall revaluate custom registries.

@injectives injectives merged commit 3aea8a0 into neo4j:5.0 Mar 8, 2022
@injectives injectives deleted the micrometer-metrics branch March 8, 2022 12:30
injectives added a commit to injectives/neo4j-java-driver that referenced this pull request Mar 8, 2022
* [WIP] Micrometer metrics instrumentation

This is a work-in-progress attempt at instrumenting the driver with Micrometer as an optional alternative metrics implementation to the existing internal one.

* Bring it to life.

* Update Micrometer metrics

This update includes general improvements, refactorings and tests.

* Make MetricsAdapter a supported, public api.

- Set scope of micrometer to provided
- Rename MetricsProvider to MetricsAdapter (it does not provide metrics, the driver does. It adapts the drivers metrics to something external)
- Make it a public interface
- Remove dependencies to other internal interfaces and classes (such as BoltServerAddress and the pool itself via ServerAddress and intsuppliers)
- Reduce visibility of internal classes as much as possible
- Make sure that adapters are not serialized
- Apply the clock to the internal adapter (an oversight from the previous iteration)

* Saving a file before commiting seems to be relevant.

* Not messing up the file while saven even more so.

* Make interfaces internal again, provide an enum to select the provider, use global registry by default.

This change keeps the existing serialization feature of config correct.
It will work for everyone just using micrometer as is with the global registry.
if there is a need to select the registriy, we can work on that later.

* Move micrometer-core optional tag to dependency declaration

* Update Config javadoc and remove imports of internal metrics classes

* Update withMetricsEnabled(boolean) implementation

* Enable ConfigBuilder.withMetricsAdapter documentation

* Update driver/src/main/java/org/neo4j/driver/ConnectionPoolMetrics.java

Co-authored-by: Gerrit Meier <meistermeier@gmail.com>

* Update ConnectionPoolMetrics documentation

* Update MetricsAdapter documentation

* Update formatting in ConfigTest

* Update wording

* Updated wording

* Formatting

* Update driver/src/main/java/org/neo4j/driver/internal/metrics/DevNullMetricsListener.java

Co-authored-by: Gerrit Meier <meistermeier@gmail.com>

* Update driver/src/main/java/org/neo4j/driver/internal/metrics/MicrometerConnectionPoolMetrics.java

Co-authored-by: Gerrit Meier <meistermeier@gmail.com>

* Fix compilation error

* Update failing test

* Fix GetConnectionPoolMetrics access

* Delete redundant counters based on review feedback

Co-authored-by: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com>
Co-authored-by: Gerrit Meier <meistermeier@gmail.com>
Co-authored-by: Michael Simons <michael.simons@neo4j.com>
@injectives injectives mentioned this pull request Mar 8, 2022
injectives added a commit that referenced this pull request Mar 9, 2022
* [WIP] Micrometer metrics instrumentation

This is a work-in-progress attempt at instrumenting the driver with Micrometer as an optional alternative metrics implementation to the existing internal one.

* Bring it to life.

* Update Micrometer metrics

This update includes general improvements, refactorings and tests.

* Make MetricsAdapter a supported, public api.

- Set scope of micrometer to provided
- Rename MetricsProvider to MetricsAdapter (it does not provide metrics, the driver does. It adapts the drivers metrics to something external)
- Make it a public interface
- Remove dependencies to other internal interfaces and classes (such as BoltServerAddress and the pool itself via ServerAddress and intsuppliers)
- Reduce visibility of internal classes as much as possible
- Make sure that adapters are not serialized
- Apply the clock to the internal adapter (an oversight from the previous iteration)

* Saving a file before commiting seems to be relevant.

* Not messing up the file while saven even more so.

* Make interfaces internal again, provide an enum to select the provider, use global registry by default.

This change keeps the existing serialization feature of config correct.
It will work for everyone just using micrometer as is with the global registry.
if there is a need to select the registriy, we can work on that later.

* Move micrometer-core optional tag to dependency declaration

* Update Config javadoc and remove imports of internal metrics classes

* Update withMetricsEnabled(boolean) implementation

* Enable ConfigBuilder.withMetricsAdapter documentation

* Update driver/src/main/java/org/neo4j/driver/ConnectionPoolMetrics.java

Co-authored-by: Gerrit Meier <meistermeier@gmail.com>

* Update ConnectionPoolMetrics documentation

* Update MetricsAdapter documentation

* Update formatting in ConfigTest

* Update wording

* Updated wording

* Formatting

* Update driver/src/main/java/org/neo4j/driver/internal/metrics/DevNullMetricsListener.java

Co-authored-by: Gerrit Meier <meistermeier@gmail.com>

* Update driver/src/main/java/org/neo4j/driver/internal/metrics/MicrometerConnectionPoolMetrics.java

Co-authored-by: Gerrit Meier <meistermeier@gmail.com>

* Fix compilation error

* Update failing test

* Fix GetConnectionPoolMetrics access

* Delete redundant counters based on review feedback

Co-authored-by: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com>
Co-authored-by: Gerrit Meier <meistermeier@gmail.com>
Co-authored-by: Michael Simons <michael.simons@neo4j.com>

Co-authored-by: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com>
Co-authored-by: Gerrit Meier <meistermeier@gmail.com>
Co-authored-by: Michael Simons <michael.simons@neo4j.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants