Implement HikariCP connection pool metrics #6003
Conversation
|
|
||
| package io.opentelemetry.javaagent.instrumentation.hikaricp; | ||
|
|
||
| import com.zaxxer.hikari.metrics.IMetricsTracker; |
There was a problem hiding this comment.
Nice - let's go ahead and add library instrumentation
There was a problem hiding this comment.
Do you mind if I do that in a separate PR? This one is not that small already, and it establishes some common code parts that might be used in other connection pill instrumentations.
There was a problem hiding this comment.
What if the library instrumentation was contributed to Hikari iteself and lived alongside the dropwizard, micrometer, and prometheus implementations?
There was a problem hiding this comment.
That could also work, probably.
The semantic conventions are not stable though, they could change in the future -- I wonder what kind of strategy we have for updating 3rd party libs when the telemetry changes.
There was a problem hiding this comment.
Splitting library out in separate PR is fine too - that being said "establishes some common code parts that might be used in other connection pill instrumentations" makes it sound like other instrumentation might come before a split, which I wouldn't recommend ;)
The semantic conventions are not stable though, they could change in the future -- I wonder what kind of strategy we have for updating 3rd party libs when the telemetry changes.
Most stable libraries will be reluctant if not forbid having a dependency on an alpha module, our -semconv module. I would probably keep upstream instrumentation off the table until the semconv is stable and part of our non-alpha artifact.
...v/src/main/java/io/opentelemetry/instrumentation/api/metrics/db/DbConnectionPoolMetrics.java
Outdated
Show resolved
Hide resolved
...v/src/main/java/io/opentelemetry/instrumentation/api/metrics/db/DbConnectionPoolMetrics.java
Show resolved
Hide resolved
...v/src/main/java/io/opentelemetry/instrumentation/api/metrics/db/DbConnectionPoolMetrics.java
Show resolved
Hide resolved
...v/src/main/java/io/opentelemetry/instrumentation/api/metrics/db/DbConnectionPoolMetrics.java
Show resolved
Hide resolved
|
|
||
| package io.opentelemetry.javaagent.instrumentation.hikaricp; | ||
|
|
||
| import com.zaxxer.hikari.metrics.IMetricsTracker; |
There was a problem hiding this comment.
What if the library instrumentation was contributed to Hikari iteself and lived alongside the dropwizard, micrometer, and prometheus implementations?
...in/java/io/opentelemetry/javaagent/instrumentation/hikaricp/OpenTelemetryMetricsTracker.java
Show resolved
Hide resolved
...in/java/io/opentelemetry/javaagent/instrumentation/hikaricp/OpenTelemetryMetricsTracker.java
Show resolved
Hide resolved
...in/java/io/opentelemetry/javaagent/instrumentation/hikaricp/OpenTelemetryMetricsTracker.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| void shouldNotBreakCustomUserMetrics() throws Exception { |
There was a problem hiding this comment.
👍🏻 good to cover this!
| point.hasAttributes( | ||
| Attributes.of(stringKey("pool.name"), poolName)))))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Favorite file formatting all week, LOL! 🙈
There was a problem hiding this comment.
This is the price we have to pay for not using Groovy 😂
There was a problem hiding this comment.
And what a price it is!
| point.hasAttributes( | ||
| Attributes.of(stringKey("pool.name"), poolName)))))); | ||
| } | ||
| } |
| @AutoService(IgnoredTypesConfigurer.class) | ||
| public class IgnoredTestTypesConfigurer implements IgnoredTypesConfigurer { | ||
|
|
||
| @Override | ||
| public void configure(Config config, IgnoredTypesBuilder builder) { | ||
| // we don't want to instrument auto-generated mocks | ||
| builder | ||
| .ignoreClass("org.mockito") | ||
| .ignoreClass("com.zaxxer.hikari.metrics.IMetricsTracker$MockitoMock$"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it's reasonable for this to be in the hikaricp module (non-testing artifact), especially since it should affect perf due to the underlying trie (as opposed to the old loop), and users may want to run their own tests using the javaagent.
(I'm also ok with it here)
| // this method is always called in the HikariPool constructor, even if the user does not | ||
| // configure anything |
* Implement HikariCP connection pool metrics * rebase after SDK update * fix muzzle * code review comments
Resolves #5860