-
Notifications
You must be signed in to change notification settings - Fork 867
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
Implement C3P0 connection pool metrics #6174
Merged
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
2c6df7d
C3P0 connection pool metrics
agoallikmaa 9d323ed
Use PooledDataSource instead of specific implementation
agoallikmaa 709a372
Add C3P0 readme
agoallikmaa 1829ee5
RuntimeException in case of underlying SQLException
agoallikmaa 36d4d2c
Use ISE instead of RuntimeException
agoallikmaa File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
plugins { | ||
id("otel.javaagent-instrumentation") | ||
} | ||
|
||
muzzle { | ||
pass { | ||
group.set("com.mchange") | ||
module.set("c3p0") | ||
versions.set("[0.9.2,)") | ||
assertInverse.set(true) | ||
// these versions have missing dependencies in maven central | ||
skip("0.9.2-pre2-RELEASE", "0.9.2-pre3") | ||
} | ||
} | ||
|
||
dependencies { | ||
library("com.mchange:c3p0:0.9.2") | ||
|
||
implementation(project(":instrumentation:c3p0-0.9:library")) | ||
|
||
testImplementation(project(":instrumentation:c3p0-0.9:testing")) | ||
} |
49 changes: 49 additions & 0 deletions
49
...telemetry/javaagent/instrumentation/c3p0/AbstractPoolBackedDataSourceInstrumentation.java
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.instrumentation.c3p0; | ||
|
||
import static io.opentelemetry.javaagent.instrumentation.c3p0.C3p0Singletons.telemetry; | ||
import static net.bytebuddy.matcher.ElementMatchers.named; | ||
|
||
import com.mchange.v2.c3p0.impl.AbstractPoolBackedDataSource; | ||
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; | ||
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; | ||
import net.bytebuddy.asm.Advice; | ||
import net.bytebuddy.description.type.TypeDescription; | ||
import net.bytebuddy.matcher.ElementMatcher; | ||
|
||
final class AbstractPoolBackedDataSourceInstrumentation implements TypeInstrumentation { | ||
|
||
@Override | ||
public ElementMatcher<TypeDescription> typeMatcher() { | ||
return named("com.mchange.v2.c3p0.impl.AbstractPoolBackedDataSource"); | ||
} | ||
|
||
@Override | ||
public void transform(TypeTransformer transformer) { | ||
transformer.applyAdviceToMethod( | ||
named("resetPoolManager"), this.getClass().getName() + "$ResetPoolManagerAdvice"); | ||
transformer.applyAdviceToMethod(named("close"), this.getClass().getName() + "$CloseAdvice"); | ||
} | ||
|
||
@SuppressWarnings("unused") | ||
public static class ResetPoolManagerAdvice { | ||
|
||
@Advice.OnMethodExit(suppress = Throwable.class) | ||
public static void onExit(@Advice.This AbstractPoolBackedDataSource dataSource) { | ||
telemetry().registerMetrics(dataSource); | ||
} | ||
} | ||
|
||
@SuppressWarnings("unused") | ||
public static class CloseAdvice { | ||
|
||
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class) | ||
public static void onExit(@Advice.This AbstractPoolBackedDataSource dataSource) { | ||
telemetry().unregisterMetrics(dataSource); | ||
} | ||
} | ||
} |
25 changes: 25 additions & 0 deletions
25
.../main/java/io/opentelemetry/javaagent/instrumentation/c3p0/C3p0InstrumentationModule.java
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.instrumentation.c3p0; | ||
|
||
import com.google.auto.service.AutoService; | ||
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; | ||
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; | ||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
@AutoService(InstrumentationModule.class) | ||
public class C3p0InstrumentationModule extends InstrumentationModule { | ||
|
||
public C3p0InstrumentationModule() { | ||
super("c3p0", "c3p0-0.9"); | ||
} | ||
|
||
@Override | ||
public List<TypeInstrumentation> typeInstrumentations() { | ||
return Collections.singletonList(new AbstractPoolBackedDataSourceInstrumentation()); | ||
} | ||
} |
21 changes: 21 additions & 0 deletions
21
...vaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/c3p0/C3p0Singletons.java
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.instrumentation.c3p0; | ||
|
||
import io.opentelemetry.api.GlobalOpenTelemetry; | ||
import io.opentelemetry.instrumentation.c3p0.C3p0Telemetry; | ||
|
||
public final class C3p0Singletons { | ||
|
||
private static final C3p0Telemetry c3p0Telemetry = | ||
C3p0Telemetry.create(GlobalOpenTelemetry.get()); | ||
|
||
public static C3p0Telemetry telemetry() { | ||
return c3p0Telemetry; | ||
} | ||
|
||
private C3p0Singletons() {} | ||
} |
29 changes: 29 additions & 0 deletions
29
...rc/test/java/io/opentelemetry/javaagent/instrumentation/c3p0/C3p0InstrumentationTest.java
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.instrumentation.c3p0; | ||
|
||
import com.mchange.v2.c3p0.PooledDataSource; | ||
import io.opentelemetry.instrumentation.c3p0.AbstractC3p0InstrumentationTest; | ||
import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension; | ||
import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension; | ||
import org.junit.jupiter.api.extension.RegisterExtension; | ||
|
||
public class C3p0InstrumentationTest extends AbstractC3p0InstrumentationTest { | ||
|
||
@RegisterExtension | ||
static final InstrumentationExtension testing = AgentInstrumentationExtension.create(); | ||
|
||
@Override | ||
protected InstrumentationExtension testing() { | ||
return testing; | ||
} | ||
|
||
@Override | ||
protected void configure(PooledDataSource dataSource) {} | ||
|
||
@Override | ||
protected void shutdown(PooledDataSource dataSource) {} | ||
} |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
# Manual Instrumentation for C3P0 | ||
|
||
Provides OpenTelemetry instrumentation for [C3P0](https://www.mchange.com/projects/c3p0/). | ||
|
||
## Quickstart | ||
|
||
### Add these dependencies to your project: | ||
|
||
Replace `OPENTELEMETRY_VERSION` with the latest stable | ||
[release](https://mvnrepository.com/artifact/io.opentelemetry). `Minimum version: 1.15.0` | ||
|
||
For Maven, add to your `pom.xml` dependencies: | ||
|
||
```xml | ||
|
||
<dependencies> | ||
<dependency> | ||
<groupId>io.opentelemetry.instrumentation</groupId> | ||
<artifactId>opentelemetry-c3p0-0.9</artifactId> | ||
<version>OPENTELEMETRY_VERSION</version> | ||
</dependency> | ||
</dependencies> | ||
``` | ||
|
||
For Gradle, add to your dependencies: | ||
|
||
```groovy | ||
implementation("io.opentelemetry.instrumentation:opentelemetry-c3p0-0.9:OPENTELEMETRY_VERSION") | ||
``` | ||
|
||
### Usage | ||
|
||
The instrumentation library allows registering `PooledDataSource` instances for | ||
collecting OpenTelemetry-based metrics. | ||
|
||
```java | ||
C3p0Telemetry c3p0Telemetry; | ||
|
||
void configure(OpenTelemetry openTelemetry, PooledDataSource dataSource) { | ||
c3p0Telemetry = C3p0Telemetry.create(openTelemetry); | ||
c3p0Telemetry.registerMetrics(dataSource); | ||
} | ||
|
||
void destroy(PooledDataSource dataSource) { | ||
c3p0Telemetry.unregisterMetrics(dataSource); | ||
} | ||
``` |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
plugins { | ||
id("otel.library-instrumentation") | ||
id("otel.nullaway-conventions") | ||
} | ||
|
||
dependencies { | ||
library("com.mchange:c3p0:0.9.2") | ||
|
||
testImplementation(project(":instrumentation:c3p0-0.9:testing")) | ||
} |
32 changes: 32 additions & 0 deletions
32
...n/c3p0-0.9/library/src/main/java/io/opentelemetry/instrumentation/c3p0/C3p0Telemetry.java
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.c3p0; | ||
|
||
import com.mchange.v2.c3p0.PooledDataSource; | ||
import io.opentelemetry.api.OpenTelemetry; | ||
|
||
public final class C3p0Telemetry { | ||
/** Returns a new {@link C3p0Telemetry} configured with the given {@link OpenTelemetry}. */ | ||
public static C3p0Telemetry create(OpenTelemetry openTelemetry) { | ||
return new C3p0Telemetry(openTelemetry); | ||
} | ||
|
||
private final OpenTelemetry openTelemetry; | ||
|
||
private C3p0Telemetry(OpenTelemetry openTelemetry) { | ||
this.openTelemetry = openTelemetry; | ||
} | ||
|
||
/** Start collecting metrics for given connection pool. */ | ||
public void registerMetrics(PooledDataSource dataSource) { | ||
ConnectionPoolMetrics.registerMetrics(openTelemetry, dataSource); | ||
} | ||
|
||
/** Stop collecting metrics for given connection pool. */ | ||
public void unregisterMetrics(PooledDataSource dataSource) { | ||
ConnectionPoolMetrics.unregisterMetrics(dataSource); | ||
} | ||
} |
122 changes: 122 additions & 0 deletions
122
....9/library/src/main/java/io/opentelemetry/instrumentation/c3p0/ConnectionPoolMetrics.java
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.c3p0; | ||
|
||
import com.mchange.v2.c3p0.PooledDataSource; | ||
import io.opentelemetry.api.OpenTelemetry; | ||
import io.opentelemetry.api.metrics.ObservableLongUpDownCounter; | ||
import io.opentelemetry.instrumentation.api.metrics.db.DbConnectionPoolMetrics; | ||
import java.sql.SQLException; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
import java.util.function.LongSupplier; | ||
import javax.annotation.Nullable; | ||
|
||
final class ConnectionPoolMetrics { | ||
private static final String INSTRUMENTATION_NAME = "io.opentelemetry.c3p0-0.9"; | ||
|
||
// a weak map does not make sense here because each Meter holds a reference to the dataSource | ||
// PooledDataSource implements equals() & hashCode() in IdentityTokenResolvable, | ||
// that's why we wrap it with IdentityDataSourceKey that uses identity comparison instead | ||
private static final Map<IdentityDataSourceKey, List<ObservableLongUpDownCounter>> | ||
dataSourceMetrics = new ConcurrentHashMap<>(); | ||
|
||
public static void registerMetrics(OpenTelemetry openTelemetry, PooledDataSource dataSource) { | ||
dataSourceMetrics.compute( | ||
new IdentityDataSourceKey(dataSource), | ||
(key, existingCounters) -> | ||
ConnectionPoolMetrics.createMeters(openTelemetry, key, existingCounters)); | ||
} | ||
|
||
private static List<ObservableLongUpDownCounter> createMeters( | ||
OpenTelemetry openTelemetry, | ||
IdentityDataSourceKey key, | ||
List<ObservableLongUpDownCounter> existingCounters) { | ||
// remove old counters from the registry in case they were already there | ||
removeMetersFromRegistry(existingCounters); | ||
|
||
PooledDataSource dataSource = key.dataSource; | ||
|
||
DbConnectionPoolMetrics metrics = | ||
DbConnectionPoolMetrics.create( | ||
openTelemetry, INSTRUMENTATION_NAME, dataSource.getDataSourceName()); | ||
|
||
return Arrays.asList( | ||
metrics.usedConnections(wrapThrowingSupplier(dataSource::getNumBusyConnectionsDefaultUser)), | ||
metrics.idleConnections(wrapThrowingSupplier(dataSource::getNumIdleConnectionsDefaultUser)), | ||
metrics.pendingRequestsForConnection( | ||
wrapThrowingSupplier(dataSource::getNumThreadsAwaitingCheckoutDefaultUser))); | ||
} | ||
|
||
public static void unregisterMetrics(PooledDataSource dataSource) { | ||
List<ObservableLongUpDownCounter> meters = | ||
dataSourceMetrics.remove(new IdentityDataSourceKey(dataSource)); | ||
removeMetersFromRegistry(meters); | ||
} | ||
|
||
private static void removeMetersFromRegistry( | ||
@Nullable List<ObservableLongUpDownCounter> observableInstruments) { | ||
if (observableInstruments != null) { | ||
for (ObservableLongUpDownCounter observable : observableInstruments) { | ||
observable.close(); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* A wrapper over {@link PooledDataSource} that implements identity comparison in its {@link | ||
* #equals(Object)} and {@link #hashCode()} methods. | ||
*/ | ||
static final class IdentityDataSourceKey { | ||
final PooledDataSource dataSource; | ||
|
||
IdentityDataSourceKey(PooledDataSource dataSource) { | ||
this.dataSource = dataSource; | ||
} | ||
|
||
@Override | ||
@SuppressWarnings("ReferenceEquality") | ||
public boolean equals(@Nullable Object o) { | ||
if (this == o) { | ||
return true; | ||
} | ||
if (o == null || getClass() != o.getClass()) { | ||
return false; | ||
} | ||
IdentityDataSourceKey that = (IdentityDataSourceKey) o; | ||
return dataSource == that.dataSource; | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return System.identityHashCode(dataSource); | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return dataSource.toString(); | ||
} | ||
} | ||
|
||
static LongSupplier wrapThrowingSupplier(DataSourceIntSupplier supplier) { | ||
return () -> { | ||
try { | ||
return supplier.getAsInt(); | ||
} catch (SQLException e) { | ||
return 0; | ||
} | ||
}; | ||
} | ||
|
||
@FunctionalInterface | ||
interface DataSourceIntSupplier { | ||
int getAsInt() throws SQLException; | ||
} | ||
|
||
private ConnectionPoolMetrics() {} | ||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jack-berg any thoughts on recording
0
vs not recording any value in this case?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking it may be better to re-throw an unchecked exception here instead of recording
0
.It looks like it will get caught and logged here: https://github.com/open-telemetry/opentelemetry-java/blob/f280f278be0222f24b7ad7a8dc7ffc293358785b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/CallbackRegistration.java#L100-L103
Or if we think this is common and don't want to log, we could consider suppressing it here:
opentelemetry-java-instrumentation/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/metrics/db/DbConnectionPoolMetrics.java
Line 65 in fae88de
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't want instrumentation to be in the habit of throwing exceptions, even if the SDK can handle it. Better to have the handling to be explicit in the callback.
I do think that recording nothing is more accurate than 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the only ways to record nothing with the current implementation of
DbConnectionPoolMetrics
would be to either throw an exception, or changeDbConnectionPoolMetrics
to use providers that return aLong
instead that can benull
. I changed it throw since I didn't see the last comment yet, but if you can suggest a better way, I can change it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you expect exception to be thrown here on a regular basis? If so, it is likely to make user logs noisy. For reference, here's the code that handles these exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked briefly at the c3p0 implementation, and I couldn't really find any case where these exceptions would really be thrown, so I think it's probably ok