From 701ac211c1d34330e0ed33b5b13c0187b7e9b127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 27 Sep 2024 11:33:59 +0200 Subject: [PATCH 1/2] fix: remove connection-id from metrics The OpenTelemetry metrics that were gathered by the JDBC driver also included a unique Connection ID for each connection. This creats too many time series. --- .../cloud/spanner/jdbc/JdbcConnection.java | 15 +++-- .../spanner/jdbc/it/ITOpenTelemetryTest.java | 59 ++++++++++++++++--- 2 files changed, 60 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java b/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java index 9b5d5e88e..ca71f5a01 100644 --- a/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java +++ b/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java @@ -91,14 +91,14 @@ class JdbcConnection extends AbstractJdbcConnection { private final Metrics metrics; - private final Attributes openTelemetryAttributes; + private final Attributes openTelemetryMetricsAttributes; JdbcConnection(String connectionUrl, ConnectionOptions options) throws SQLException { super(connectionUrl, options); this.useLegacyIsValidCheck = useLegacyValidCheck(); OpenTelemetry openTelemetry = getSpanner().getOptions().getOpenTelemetry(); - this.openTelemetryAttributes = - createOpenTelemetryAttributes(getConnectionOptions().getDatabaseId()); + this.openTelemetryMetricsAttributes = + createOpenTelemetryAttributes(getConnectionOptions().getDatabaseId(), false); this.metrics = new Metrics(openTelemetry); } @@ -114,9 +114,12 @@ static boolean useLegacyValidCheck() { } @VisibleForTesting - static Attributes createOpenTelemetryAttributes(DatabaseId databaseId) { + static Attributes createOpenTelemetryAttributes(DatabaseId databaseId, boolean includeConnectionId) { AttributesBuilder attributesBuilder = Attributes.builder(); - attributesBuilder.put("connection_id", UUID.randomUUID().toString()); + // A unique connection ID should only be included for tracing and not for metrics. + if (includeConnectionId) { + attributesBuilder.put("connection_id", UUID.randomUUID().toString()); + } attributesBuilder.put("database", databaseId.getDatabase()); attributesBuilder.put("instance_id", databaseId.getInstanceId().getInstance()); attributesBuilder.put("project_id", databaseId.getInstanceId().getProject()); @@ -124,7 +127,7 @@ static Attributes createOpenTelemetryAttributes(DatabaseId databaseId) { } public void recordClientLibLatencyMetric(long value) { - metrics.recordClientLibLatency(value, openTelemetryAttributes); + metrics.recordClientLibLatency(value, openTelemetryMetricsAttributes); } @Override diff --git a/src/test/java/com/google/cloud/spanner/jdbc/it/ITOpenTelemetryTest.java b/src/test/java/com/google/cloud/spanner/jdbc/it/ITOpenTelemetryTest.java index bbbbaf344..812e483f3 100644 --- a/src/test/java/com/google/cloud/spanner/jdbc/it/ITOpenTelemetryTest.java +++ b/src/test/java/com/google/cloud/spanner/jdbc/it/ITOpenTelemetryTest.java @@ -51,10 +51,17 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; +import java.util.ArrayList; +import java.util.List; import java.util.Properties; import java.util.UUID; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeUnit; +import java.util.function.Supplier; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; @@ -125,13 +132,14 @@ public void setup() { } @Test - public void testGlobalOpenTelemetry() throws SQLException, IOException, InterruptedException { + public void testGlobalOpenTelemetry() throws Exception { GlobalOpenTelemetry.resetForTest(); GlobalOpenTelemetry.set(openTelemetry); Properties info = new Properties(); info.put("enableExtendedTracing", "true"); try (Connection connection = createConnection(env, database, info)) { testOpenTelemetry(connection); + testOpenTelemetryConcurrency(() -> createConnection(env, database, info)); } finally { GlobalOpenTelemetry.resetForTest(); } @@ -139,7 +147,7 @@ public void testGlobalOpenTelemetry() throws SQLException, IOException, Interrup @Test public void testOpenTelemetryInProperties() - throws SQLException, IOException, InterruptedException { + throws Exception { // Make sure there is no Global OpenTelemetry. GlobalOpenTelemetry.resetForTest(); Properties info = new Properties(); @@ -148,6 +156,7 @@ public void testOpenTelemetryInProperties() try (Connection connection = createConnection(env, database, info)) { testOpenTelemetry(connection); } + testOpenTelemetryConcurrency(() -> createConnection(env, database, info)); } private void testOpenTelemetry(Connection connection) @@ -159,14 +168,14 @@ private void testOpenTelemetry(Connection connection) // Test executeQuery(String) try (ResultSet resultSet = statement.executeQuery(sql)) { - assertQueryResult(resultSet, sql, uuid); + assertQueryResult(resultSet, sql, uuid, true); } // Test execute(String) uuid = UUID.randomUUID(); sql = "select '" + uuid + "'"; assertTrue(statement.execute(sql)); - assertQueryResult(statement.getResultSet(), sql, uuid); + assertQueryResult(statement.getResultSet(), sql, uuid, true); // Test executeUpdate(String) uuid = UUID.randomUUID(); @@ -189,7 +198,7 @@ private void testOpenTelemetry(Connection connection) String sql = "select '" + uuid + "'"; try (PreparedStatement statement = connection.prepareStatement(sql)) { try (ResultSet resultSet = statement.executeQuery()) { - assertQueryResult(resultSet, sql, uuid); + assertQueryResult(resultSet, sql, uuid, true); } } @@ -197,7 +206,7 @@ private void testOpenTelemetry(Connection connection) sql = "select '" + uuid + "'"; try (PreparedStatement statement = connection.prepareStatement(sql)) { assertTrue(statement.execute()); - assertQueryResult(statement.getResultSet(), sql, uuid); + assertQueryResult(statement.getResultSet(), sql, uuid, true); } uuid = UUID.randomUUID(); @@ -216,15 +225,49 @@ private void testOpenTelemetry(Connection connection) assertUpdateResult(statement.executeLargeUpdate(), spannerSql); } } + + private interface ConnectionProducer { + Connection createConnection() throws SQLException; + } + + private void testOpenTelemetryConcurrency(ConnectionProducer connectionProducer) throws Exception { + int numThreads = 16; + int numIterations = 1000; + ExecutorService executor = Executors.newFixedThreadPool(16); + List> futures = new ArrayList<>(numThreads); + for (int n=0; n) () -> { + try (Connection connection = connectionProducer.createConnection(); Statement statement = connection.createStatement()) { + for (int i = 0; i < numIterations; i++) { + UUID uuid = UUID.randomUUID(); + String sql = "select '" + uuid + "'"; - private void assertQueryResult(ResultSet resultSet, String sql, UUID uuid) + try (ResultSet resultSet = statement.executeQuery(sql)) { + assertQueryResult(resultSet, sql, uuid, false); + } + } + } + return null; + })); + } + executor.shutdown(); + assertTrue(executor.awaitTermination(600L, TimeUnit.SECONDS)); + for (Future future : futures) { + // Just verify that we did not get an exception. + future.get(); + } + } + + private void assertQueryResult(ResultSet resultSet, String sql, UUID uuid, boolean assertTrace) throws SQLException, IOException, InterruptedException { assertTrue(resultSet.next()); assertEquals(uuid.toString(), resultSet.getString(1)); assertFalse(resultSet.next()); flushOpenTelemetry(); - assertTrace(sql); + if (assertTrace) { + assertTrace(sql); + } } private void assertUpdateResult(long updateCount, String sql) From f968e23dcc5649410b068beb257da4b55f199c88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Fri, 27 Sep 2024 11:40:07 +0200 Subject: [PATCH 2/2] chore: run code formatter --- .../cloud/spanner/jdbc/JdbcConnection.java | 3 +- .../spanner/jdbc/it/ITOpenTelemetryTest.java | 43 ++++++++++--------- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java b/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java index ca71f5a01..26b381dd2 100644 --- a/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java +++ b/src/main/java/com/google/cloud/spanner/jdbc/JdbcConnection.java @@ -114,7 +114,8 @@ static boolean useLegacyValidCheck() { } @VisibleForTesting - static Attributes createOpenTelemetryAttributes(DatabaseId databaseId, boolean includeConnectionId) { + static Attributes createOpenTelemetryAttributes( + DatabaseId databaseId, boolean includeConnectionId) { AttributesBuilder attributesBuilder = Attributes.builder(); // A unique connection ID should only be included for tracing and not for metrics. if (includeConnectionId) { diff --git a/src/test/java/com/google/cloud/spanner/jdbc/it/ITOpenTelemetryTest.java b/src/test/java/com/google/cloud/spanner/jdbc/it/ITOpenTelemetryTest.java index 812e483f3..83cfab81c 100644 --- a/src/test/java/com/google/cloud/spanner/jdbc/it/ITOpenTelemetryTest.java +++ b/src/test/java/com/google/cloud/spanner/jdbc/it/ITOpenTelemetryTest.java @@ -61,7 +61,6 @@ import java.util.concurrent.Future; import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeUnit; -import java.util.function.Supplier; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; @@ -146,8 +145,7 @@ public void testGlobalOpenTelemetry() throws Exception { } @Test - public void testOpenTelemetryInProperties() - throws Exception { + public void testOpenTelemetryInProperties() throws Exception { // Make sure there is no Global OpenTelemetry. GlobalOpenTelemetry.resetForTest(); Properties info = new Properties(); @@ -225,30 +223,35 @@ private void testOpenTelemetry(Connection connection) assertUpdateResult(statement.executeLargeUpdate(), spannerSql); } } - + private interface ConnectionProducer { Connection createConnection() throws SQLException; } - - private void testOpenTelemetryConcurrency(ConnectionProducer connectionProducer) throws Exception { + + private void testOpenTelemetryConcurrency(ConnectionProducer connectionProducer) + throws Exception { int numThreads = 16; int numIterations = 1000; ExecutorService executor = Executors.newFixedThreadPool(16); List> futures = new ArrayList<>(numThreads); - for (int n=0; n) () -> { - try (Connection connection = connectionProducer.createConnection(); Statement statement = connection.createStatement()) { - for (int i = 0; i < numIterations; i++) { - UUID uuid = UUID.randomUUID(); - String sql = "select '" + uuid + "'"; - - try (ResultSet resultSet = statement.executeQuery(sql)) { - assertQueryResult(resultSet, sql, uuid, false); - } - } - } - return null; - })); + for (int n = 0; n < numThreads; n++) { + futures.add( + executor.submit( + (Callable) + () -> { + try (Connection connection = connectionProducer.createConnection(); + Statement statement = connection.createStatement()) { + for (int i = 0; i < numIterations; i++) { + UUID uuid = UUID.randomUUID(); + String sql = "select '" + uuid + "'"; + + try (ResultSet resultSet = statement.executeQuery(sql)) { + assertQueryResult(resultSet, sql, uuid, false); + } + } + } + return null; + })); } executor.shutdown(); assertTrue(executor.awaitTermination(600L, TimeUnit.SECONDS));