From 786c6e6318c0945f818f1498545efeba43fb4b05 Mon Sep 17 00:00:00 2001 From: Eli Levine Date: Thu, 11 Sep 2014 12:17:54 -0700 Subject: [PATCH 1/3] First pass at custom tracing annotations --- .../phoenix/trace/BaseTracingTestIT.java | 12 +++- .../trace/PhoenixTracingEndToEndIT.java | 66 ++++++++++++++++++- .../phoenix/jdbc/PhoenixConnection.java | 9 +++ .../apache/phoenix/query/QueryServices.java | 1 + .../apache/phoenix/trace/util/Tracing.java | 19 ++++++ .../org/apache/phoenix/util/JDBCUtil.java | 42 +++++++++++- .../org/apache/phoenix/util/JDBCUtilTest.java | 59 +++++++++++++++++ .../apache/phoenix/trace/TracingCompat.java | 2 +- 8 files changed, 205 insertions(+), 5 deletions(-) create mode 100644 phoenix-core/src/test/java/org/apache/phoenix/util/JDBCUtilTest.java diff --git a/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java b/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java index 1f4990be856..a1d194645d9 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java @@ -22,6 +22,8 @@ import java.sql.Connection; import java.sql.DriverManager; import java.sql.SQLException; +import java.util.HashMap; +import java.util.Map; import java.util.Properties; import org.apache.commons.logging.Log; @@ -32,6 +34,7 @@ import org.apache.phoenix.metrics.PhoenixAbstractMetric; import org.apache.phoenix.metrics.PhoenixMetricTag; import org.apache.phoenix.metrics.PhoenixMetricsRecord; +import org.apache.phoenix.query.QueryServices; import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.schema.TableNotFoundException; import org.apache.phoenix.trace.util.Tracing; @@ -86,9 +89,16 @@ public static Connection getConnectionWithoutTracing(Properties props) throws SQ conn.setAutoCommit(false); return conn; } + + public static Connection getTracingConnection() throws Exception { + return getTracingConnection(new HashMap(0)); + } - public static Connection getTracingConnection() throws Exception { + public static Connection getTracingConnection(Map customAnnotations) throws Exception { Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); + for (Map.Entry annot : customAnnotations.entrySet()) { + props.put(QueryServices.TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX + annot.getKey(), annot.getValue()); + } return getConnectionWithTracingFrequency(props, Tracing.Frequency.ALWAYS); } diff --git a/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java b/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java index 6742f9e595e..2aeb28b6731 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java @@ -17,7 +17,9 @@ */ package org.apache.phoenix.trace; +import static org.apache.phoenix.query.QueryServicesOptions.DEFAULT_TRACING_STATS_TABLE_NAME; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.sql.Connection; @@ -49,6 +51,8 @@ import org.junit.experimental.categories.Category; import org.junit.runner.RunWith; +import com.google.common.collect.ImmutableMap; + /** * Test that the logging sink stores the expected metrics/stats */ @@ -257,7 +261,7 @@ private void createTestTable(Connection conn, boolean withIndex) throws SQLExcep @Test public void testScanTracing() throws Exception { // separate connections to minimize amount of traces that are generated - Connection traceable = getTracingConnection(); + Connection traceable = getTracingConnection(ImmutableMap.of("annot1", "v1")); Connection conn = getConnectionWithoutTracing(); // one call for client side, one call for server side @@ -353,7 +357,67 @@ public boolean foundTrace(TraceHolder trace) { }); assertTrue("Didn't find the parallel scanner in the tracing", found); } + + @Test + public void testCustomAnnotationTracing() throws Exception { + final String customAnnotationKey = "myannot"; + final String customAnnotationValue = "a1"; + // separate connections to minimize amount of traces that are generated + Connection traceable = getTracingConnection(ImmutableMap.of(customAnnotationKey, customAnnotationValue)); + Connection conn = getConnectionWithoutTracing(); + + // one call for client side, one call for server side + CountDownLatch updated = new CountDownLatch(2); + waitForCommit(updated); + + // create a dummy table + createTestTable(conn, false); + // update the table, but don't trace these, to simplify the traces we read + LOG.debug("Doing dummy the writes to the tracked table"); + String insert = "UPSERT INTO " + table + " VALUES (?, ?)"; + PreparedStatement stmt = conn.prepareStatement(insert); + stmt.setString(1, "key1"); + stmt.setLong(2, 1); + stmt.execute(); + conn.commit(); + conn.rollback(); + + // setup for next set of updates + stmt.setString(1, "key2"); + stmt.setLong(2, 2); + stmt.execute(); + conn.commit(); + conn.rollback(); + + // do a scan of the table + String read = "SELECT * FROM " + table; + ResultSet results = traceable.createStatement().executeQuery(read); + assertTrue("Didn't get first result", results.next()); + assertTrue("Didn't get second result", results.next()); + results.close(); + + assertTrue("Get expected updates to trace table", updated.await(200, TimeUnit.SECONDS)); + + boolean tracingComplete = checkStoredTraces(conn, new TraceChecker(){ + @Override + public boolean foundTrace(TraceHolder currentTrace) { + return currentTrace.toString().contains(customAnnotationKey); + } + }); + + assertTrue("Didn't find the custom annotation in the tracing", tracingComplete); + + ResultSet rs = conn.createStatement().executeQuery( + "select annotations.a0, " + PhoenixTableMetricsWriter.ANNOTATION_COUNT + + " from " + DEFAULT_TRACING_STATS_TABLE_NAME + "(annotations.a0 varchar)" + + " where annotations.a0 like '%" + customAnnotationKey + "%'"); + + assertTrue("Didn't find the custom annotation in the tracing", rs.next()); + assertTrue(rs.getString(1).contains(customAnnotationValue)); + assertFalse("Expected only one line with custom annotation but found more", rs.next()); + } + private boolean checkStoredTraces(Connection conn, TraceChecker checker) throws Exception { TraceReader reader = new TraceReader(conn); int retries = 0; diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java index 622fc8a739b..14a24aae451 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java @@ -17,6 +17,8 @@ */ package org.apache.phoenix.jdbc; +import static java.util.Collections.emptyMap; + import java.io.EOFException; import java.io.IOException; import java.io.PrintStream; @@ -86,6 +88,7 @@ import com.google.common.base.Objects; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -120,6 +123,7 @@ public class PhoenixConnection implements Connection, org.apache.phoenix.jdbc.Jd private boolean isClosed = false; private Sampler sampler; private boolean readOnly = false; + private Map customTracingAnnotations = emptyMap(); static { Tracing.addTraceMetricsSource(); @@ -214,11 +218,16 @@ public boolean prune(PTable table) { // setup tracing, if its enabled this.sampler = Tracing.getConfiguredSampler(this); + this.customTracingAnnotations = ImmutableMap.copyOf(JDBCUtil.getCustomTracingAnnotations(url, info)); } public Sampler getSampler() { return this.sampler; } + + public Map getCustomTracingAnnotations() { + return customTracingAnnotations; + } public int executeStatements(Reader reader, List binds, PrintStream out) throws IOException, SQLException { int bindsOffset = 0; diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java b/phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java index 95ec53b3d50..c42f67e32dd 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java @@ -132,6 +132,7 @@ public interface QueryServices extends SQLCloseable { public static final String TRACING_PAGE_SIZE_ATTRIB = "phoenix.trace.read.pagesize"; public static final String TRACING_PROBABILITY_THRESHOLD_ATTRIB = "phoenix.trace.probability.threshold"; public static final String TRACING_STATS_TABLE_NAME_ATTRIB = "phoenix.trace.statsTableName"; + public static final String TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX = "phoenix.trace.custom.annotation."; public static final String USE_REVERSE_SCAN_ATTRIB = "phoenix.query.useReverseScan"; diff --git a/phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java b/phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java index f1926f87662..458a2df5dd4 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java @@ -17,9 +17,12 @@ */ package org.apache.phoenix.trace.util; +import java.util.Map; import java.util.Properties; import java.util.concurrent.Callable; +import javax.annotation.Nullable; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; @@ -32,6 +35,7 @@ import org.apache.phoenix.query.QueryServices; import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.trace.TracingCompat; +import org.apache.phoenix.util.StringUtil; import org.cloudera.htrace.Sampler; import org.cloudera.htrace.Span; import org.cloudera.htrace.Trace; @@ -43,6 +47,8 @@ import org.cloudera.htrace.wrappers.TraceRunnable; import com.google.common.base.Function; +import com.google.common.base.Preconditions; +import com.sun.istack.NotNull; /** * Helper class to manage using the {@link Tracer} within Phoenix @@ -153,6 +159,7 @@ public static void setSampling(Properties props, Frequency freq) { public static TraceScope startNewSpan(PhoenixConnection connection, String string) { Sampler sampler = connection.getSampler(); TraceScope scope = Trace.startSpan(string, sampler); + addCustomAnnotationsToSpan(scope.getSpan(), connection); return scope; } @@ -257,6 +264,18 @@ public static Callable wrap(Callable callable, String description) { public static CallWrapper withTracing(PhoenixConnection conn, String desc) { return new TracingWrapper(conn, desc); } + + private static void addCustomAnnotationsToSpan(@Nullable Span span, @NotNull PhoenixConnection conn) { + Preconditions.checkNotNull(conn); + + if (span != null) { + Map annotations = conn.getCustomTracingAnnotations(); + // copy over the annotations as bytes + for (Map.Entry annotation : annotations.entrySet()) { + span.addKVAnnotation(StringUtil.toBytes(annotation.getKey()), StringUtil.toBytes(annotation.getValue())); + } + } + } private static class TracingWrapper implements CallWrapper { private TraceScope scope; diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/JDBCUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/util/JDBCUtil.java index 6148417f2d2..db81cf1a2b4 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/util/JDBCUtil.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/util/JDBCUtil.java @@ -17,7 +17,11 @@ */ package org.apache.phoenix.util; +import static com.google.common.collect.Maps.newHashMapWithExpectedSize; +import static org.apache.phoenix.query.QueryServices.TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX; + import java.sql.SQLException; +import java.util.Map; import java.util.Properties; import javax.annotation.Nullable; @@ -27,13 +31,14 @@ import org.apache.phoenix.schema.PName; import org.apache.phoenix.schema.PNameFactory; +import com.google.common.base.Preconditions; +import com.sun.istack.NotNull; + /** * Utilities for JDBC * - * - * @since 178 */ public class JDBCUtil { @@ -63,6 +68,39 @@ public static String findProperty(String url, Properties info, String propName) } return propValue; } + + private static Map getCombinedConnectionProperties(String url, Properties info) { + Map result = newHashMapWithExpectedSize(info.size()); + for (String propName : info.stringPropertyNames()) { + result.put(propName, info.getProperty(propName)); + } + String[] urlPropNameValues = url.split(";"); + if (urlPropNameValues.length > 1) { + for (int i = 1; i < urlPropNameValues.length; i++) { + String[] urlPropNameValue = urlPropNameValues[i].split("="); + if (urlPropNameValue.length == 2) { + result.put(urlPropNameValue[0], urlPropNameValue[1]); + } + } + } + + return result; + } + + public static Map getCustomTracingAnnotations(@NotNull String url, @NotNull Properties info) { + Preconditions.checkNotNull(url); + Preconditions.checkNotNull(info); + + Map combinedProperties = getCombinedConnectionProperties(url, info); + Map result = newHashMapWithExpectedSize(combinedProperties.size()); + for (Map.Entry prop : combinedProperties.entrySet()) { + if (prop.getKey().startsWith(TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX) && + prop.getKey().length() > TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX.length()) { + result.put(prop.getKey().substring(TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX.length()), prop.getValue()); + } + } + return result; + } public static Long getCurrentSCN(String url, Properties info) throws SQLException { String scnStr = findProperty(url, info, PhoenixRuntime.CURRENT_SCN_ATTRIB); diff --git a/phoenix-core/src/test/java/org/apache/phoenix/util/JDBCUtilTest.java b/phoenix-core/src/test/java/org/apache/phoenix/util/JDBCUtilTest.java new file mode 100644 index 00000000000..5783bfb6ed2 --- /dev/null +++ b/phoenix-core/src/test/java/org/apache/phoenix/util/JDBCUtilTest.java @@ -0,0 +1,59 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.util; + +import static org.apache.phoenix.query.QueryServices.TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.util.Map; +import java.util.Properties; + +import org.junit.Test; + +public class JDBCUtilTest { + + @Test + public void testGetCustomTracingAnnotationsWithNone() { + String url = "localhost;TenantId=abc;"; + Map customAnnotations = JDBCUtil.getCustomTracingAnnotations(url, new Properties()); + assertTrue(customAnnotations.isEmpty()); + } + + @Test + public void testGetCustomTracingAnnotationInBothPropertiesAndURL() { + String annotKey1 = "key1"; + String annotVal1 = "val1"; + String annotKey2 = "key2"; + String annotVal2 = "val2"; + String annotKey3 = "key3"; + String annotVal3 = "val3"; + + String url= "localhost;" + TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX + annotKey1 + '=' + annotVal1; + + Properties prop = new Properties(); + prop.put(TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX + annotKey2, annotVal2); + prop.put(TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX + annotKey3, annotVal3); + + Map customAnnotations = JDBCUtil.getCustomTracingAnnotations(url, prop); + assertEquals(3, customAnnotations.size()); + assertEquals(annotVal1, customAnnotations.get(annotKey1)); + assertEquals(annotVal2, customAnnotations.get(annotKey2)); + assertEquals(annotVal3, customAnnotations.get(annotKey3)); + } +} diff --git a/phoenix-hadoop-compat/src/main/java/org/apache/phoenix/trace/TracingCompat.java b/phoenix-hadoop-compat/src/main/java/org/apache/phoenix/trace/TracingCompat.java index 783bfd62cee..3f81c2cad5f 100644 --- a/phoenix-hadoop-compat/src/main/java/org/apache/phoenix/trace/TracingCompat.java +++ b/phoenix-hadoop-compat/src/main/java/org/apache/phoenix/trace/TracingCompat.java @@ -53,7 +53,7 @@ public static void addAnnotation(Span span, String message, int value) { } public static Pair readAnnotation(byte[] key, byte[] value) { - return new Pair(new String(key), Integer.toString(Bytes.toInt(value))); + return new Pair(new String(key), new String(value)); } public static MetricsWriter initializeWriter(String clazz) { From 977b701d89d192e226773ad7a4ec76bf5091c359 Mon Sep 17 00:00:00 2001 From: Eli Levine Date: Tue, 16 Sep 2014 11:51:32 -0700 Subject: [PATCH 2/3] Automatically add TenantId and CurrentSCN property as custom tracing annotations --- .../phoenix/trace/BaseTracingTestIT.java | 8 +++++-- .../trace/PhoenixTracingEndToEndIT.java | 22 +++++++++++++------ .../phoenix/jdbc/PhoenixConnection.java | 15 ++++++++++++- 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java b/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java index a1d194645d9..7c5628ec82d 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java @@ -39,6 +39,7 @@ import org.apache.phoenix.schema.TableNotFoundException; import org.apache.phoenix.trace.util.Tracing; import org.apache.phoenix.trace.util.Tracing.Frequency; +import org.apache.phoenix.util.PhoenixRuntime; import org.apache.phoenix.util.PropertiesUtil; import org.junit.Before; @@ -91,14 +92,17 @@ public static Connection getConnectionWithoutTracing(Properties props) throws SQ } public static Connection getTracingConnection() throws Exception { - return getTracingConnection(new HashMap(0)); + return getTracingConnection(new HashMap(0), null); } - public static Connection getTracingConnection(Map customAnnotations) throws Exception { + public static Connection getTracingConnection(Map customAnnotations, String tenantId) throws Exception { Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); for (Map.Entry annot : customAnnotations.entrySet()) { props.put(QueryServices.TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX + annot.getKey(), annot.getValue()); } + if (tenantId != null) { + props.put(PhoenixRuntime.TENANT_ID_ATTRIB, tenantId); + } return getConnectionWithTracingFrequency(props, Tracing.Frequency.ALWAYS); } diff --git a/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java b/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java index 2aeb28b6731..847b58284e9 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java @@ -18,6 +18,7 @@ package org.apache.phoenix.trace; import static org.apache.phoenix.query.QueryServicesOptions.DEFAULT_TRACING_STATS_TABLE_NAME; +import static org.apache.phoenix.util.PhoenixRuntime.TENANT_ID_ATTRIB; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -261,7 +262,7 @@ private void createTestTable(Connection conn, boolean withIndex) throws SQLExcep @Test public void testScanTracing() throws Exception { // separate connections to minimize amount of traces that are generated - Connection traceable = getTracingConnection(ImmutableMap.of("annot1", "v1")); + Connection traceable = getTracingConnection(); Connection conn = getConnectionWithoutTracing(); // one call for client side, one call for server side @@ -362,8 +363,9 @@ public boolean foundTrace(TraceHolder trace) { public void testCustomAnnotationTracing() throws Exception { final String customAnnotationKey = "myannot"; final String customAnnotationValue = "a1"; + final String tenantId = "tenant1"; // separate connections to minimize amount of traces that are generated - Connection traceable = getTracingConnection(ImmutableMap.of(customAnnotationKey, customAnnotationValue)); + Connection traceable = getTracingConnection(ImmutableMap.of(customAnnotationKey, customAnnotationValue), tenantId); Connection conn = getConnectionWithoutTracing(); // one call for client side, one call for server side @@ -399,22 +401,28 @@ public void testCustomAnnotationTracing() throws Exception { assertTrue("Get expected updates to trace table", updated.await(200, TimeUnit.SECONDS)); + assertAnnotationPresent(TENANT_ID_ATTRIB, tenantId, 0, conn); + assertAnnotationPresent(customAnnotationKey, customAnnotationValue, 1, conn); + // CurrentSCN is also added as an annotation. Not tested here because it screws up test setup. + } + + private void assertAnnotationPresent(final String annotationKey, String annotationValue, int annotationIndex, Connection conn) throws Exception { boolean tracingComplete = checkStoredTraces(conn, new TraceChecker(){ @Override public boolean foundTrace(TraceHolder currentTrace) { - return currentTrace.toString().contains(customAnnotationKey); + return currentTrace.toString().contains(annotationKey); } }); assertTrue("Didn't find the custom annotation in the tracing", tracingComplete); ResultSet rs = conn.createStatement().executeQuery( - "select annotations.a0, " + PhoenixTableMetricsWriter.ANNOTATION_COUNT - + " from " + DEFAULT_TRACING_STATS_TABLE_NAME + "(annotations.a0 varchar)" - + " where annotations.a0 like '%" + customAnnotationKey + "%'"); + "select annotations.a" + annotationIndex + + " from " + DEFAULT_TRACING_STATS_TABLE_NAME + "(annotations.a" + annotationIndex + " varchar)" + + " where annotations.a" + annotationIndex + " like '%" + annotationKey + "%'"); assertTrue("Didn't find the custom annotation in the tracing", rs.next()); - assertTrue(rs.getString(1).contains(customAnnotationValue)); + assertTrue(rs.getString(1).contains(annotationValue)); assertFalse("Expected only one line with custom annotation but found more", rs.next()); } diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java index 14a24aae451..ed022b48225 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java @@ -89,6 +89,7 @@ import com.google.common.base.Objects; import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableMap.Builder; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -218,7 +219,19 @@ public boolean prune(PTable table) { // setup tracing, if its enabled this.sampler = Tracing.getConfiguredSampler(this); - this.customTracingAnnotations = ImmutableMap.copyOf(JDBCUtil.getCustomTracingAnnotations(url, info)); + this.customTracingAnnotations = getImmutableCustomTracingAnnotations(); + } + + private ImmutableMap getImmutableCustomTracingAnnotations() { + Builder result = ImmutableMap.builder(); + result.putAll(JDBCUtil.getCustomTracingAnnotations(url, info)); + if (getSCN() != null) { + result.put(PhoenixRuntime.CURRENT_SCN_ATTRIB, getSCN().toString()); + } + if (getTenantId() != null) { + result.put(PhoenixRuntime.TENANT_ID_ATTRIB, getTenantId().getString()); + } + return result.build(); } public Sampler getSampler() { From dd13307478860940fd80085fbce16af208cb20f5 Mon Sep 17 00:00:00 2001 From: Eli Levine Date: Thu, 18 Sep 2014 19:07:06 -0700 Subject: [PATCH 3/3] Addressing comments from code review --- .../phoenix/trace/BaseTracingTestIT.java | 8 ++++---- .../trace/PhoenixTracingEndToEndIT.java | 19 ++++--------------- .../phoenix/jdbc/PhoenixConnection.java | 2 +- .../apache/phoenix/query/QueryServices.java | 1 - .../apache/phoenix/trace/util/Tracing.java | 16 +++++++++------- .../org/apache/phoenix/util/JDBCUtil.java | 13 ++++++++----- .../apache/phoenix/util/PhoenixRuntime.java | 7 +++++++ .../org/apache/phoenix/util/JDBCUtilTest.java | 12 ++++++------ .../apache/phoenix/trace/TracingCompat.java | 2 +- 9 files changed, 40 insertions(+), 40 deletions(-) diff --git a/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java b/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java index 7c5628ec82d..92b22508a63 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java @@ -17,12 +17,13 @@ */ package org.apache.phoenix.trace; +import static org.apache.phoenix.util.PhoenixRuntime.ANNOTATION_ATTRIB_PREFIX; import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES; import java.sql.Connection; import java.sql.DriverManager; import java.sql.SQLException; -import java.util.HashMap; +import java.util.Collections; import java.util.Map; import java.util.Properties; @@ -34,7 +35,6 @@ import org.apache.phoenix.metrics.PhoenixAbstractMetric; import org.apache.phoenix.metrics.PhoenixMetricTag; import org.apache.phoenix.metrics.PhoenixMetricsRecord; -import org.apache.phoenix.query.QueryServices; import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.schema.TableNotFoundException; import org.apache.phoenix.trace.util.Tracing; @@ -92,13 +92,13 @@ public static Connection getConnectionWithoutTracing(Properties props) throws SQ } public static Connection getTracingConnection() throws Exception { - return getTracingConnection(new HashMap(0), null); + return getTracingConnection(Collections.emptyMap(), null); } public static Connection getTracingConnection(Map customAnnotations, String tenantId) throws Exception { Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); for (Map.Entry annot : customAnnotations.entrySet()) { - props.put(QueryServices.TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX + annot.getKey(), annot.getValue()); + props.put(ANNOTATION_ATTRIB_PREFIX + annot.getKey(), annot.getValue()); } if (tenantId != null) { props.put(PhoenixRuntime.TENANT_ID_ATTRIB, tenantId); diff --git a/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java b/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java index 847b58284e9..0fc80ed77f7 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java @@ -17,10 +17,8 @@ */ package org.apache.phoenix.trace; -import static org.apache.phoenix.query.QueryServicesOptions.DEFAULT_TRACING_STATS_TABLE_NAME; import static org.apache.phoenix.util.PhoenixRuntime.TENANT_ID_ATTRIB; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.sql.Connection; @@ -401,29 +399,20 @@ public void testCustomAnnotationTracing() throws Exception { assertTrue("Get expected updates to trace table", updated.await(200, TimeUnit.SECONDS)); - assertAnnotationPresent(TENANT_ID_ATTRIB, tenantId, 0, conn); - assertAnnotationPresent(customAnnotationKey, customAnnotationValue, 1, conn); + assertAnnotationPresent(customAnnotationKey, customAnnotationValue, conn); + assertAnnotationPresent(TENANT_ID_ATTRIB, tenantId, conn); // CurrentSCN is also added as an annotation. Not tested here because it screws up test setup. } - private void assertAnnotationPresent(final String annotationKey, String annotationValue, int annotationIndex, Connection conn) throws Exception { + private void assertAnnotationPresent(final String annotationKey, final String annotationValue, Connection conn) throws Exception { boolean tracingComplete = checkStoredTraces(conn, new TraceChecker(){ @Override public boolean foundTrace(TraceHolder currentTrace) { - return currentTrace.toString().contains(annotationKey); + return currentTrace.toString().contains(annotationKey + " - " + annotationValue); } }); assertTrue("Didn't find the custom annotation in the tracing", tracingComplete); - - ResultSet rs = conn.createStatement().executeQuery( - "select annotations.a" + annotationIndex - + " from " + DEFAULT_TRACING_STATS_TABLE_NAME + "(annotations.a" + annotationIndex + " varchar)" - + " where annotations.a" + annotationIndex + " like '%" + annotationKey + "%'"); - - assertTrue("Didn't find the custom annotation in the tracing", rs.next()); - assertTrue(rs.getString(1).contains(annotationValue)); - assertFalse("Expected only one line with custom annotation but found more", rs.next()); } private boolean checkStoredTraces(Connection conn, TraceChecker checker) throws Exception { diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java index ed022b48225..9a010188075 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java @@ -224,7 +224,7 @@ public boolean prune(PTable table) { private ImmutableMap getImmutableCustomTracingAnnotations() { Builder result = ImmutableMap.builder(); - result.putAll(JDBCUtil.getCustomTracingAnnotations(url, info)); + result.putAll(JDBCUtil.getAnnotations(url, info)); if (getSCN() != null) { result.put(PhoenixRuntime.CURRENT_SCN_ATTRIB, getSCN().toString()); } diff --git a/phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java b/phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java index a352f3a6136..9594f33b79d 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java @@ -132,7 +132,6 @@ public interface QueryServices extends SQLCloseable { public static final String TRACING_PAGE_SIZE_ATTRIB = "phoenix.trace.read.pagesize"; public static final String TRACING_PROBABILITY_THRESHOLD_ATTRIB = "phoenix.trace.probability.threshold"; public static final String TRACING_STATS_TABLE_NAME_ATTRIB = "phoenix.trace.statsTableName"; - public static final String TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX = "phoenix.trace.custom.annotation."; public static final String USE_REVERSE_SCAN_ATTRIB = "phoenix.query.useReverseScan"; diff --git a/phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java b/phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java index 458a2df5dd4..c7c381eeaf0 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java @@ -17,6 +17,8 @@ */ package org.apache.phoenix.trace.util; +import static org.apache.phoenix.util.StringUtil.toBytes; + import java.util.Map; import java.util.Properties; import java.util.concurrent.Callable; @@ -35,7 +37,6 @@ import org.apache.phoenix.query.QueryServices; import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.trace.TracingCompat; -import org.apache.phoenix.util.StringUtil; import org.cloudera.htrace.Sampler; import org.cloudera.htrace.Span; import org.cloudera.htrace.Trace; @@ -268,12 +269,13 @@ public static CallWrapper withTracing(PhoenixConnection conn, String desc) { private static void addCustomAnnotationsToSpan(@Nullable Span span, @NotNull PhoenixConnection conn) { Preconditions.checkNotNull(conn); - if (span != null) { - Map annotations = conn.getCustomTracingAnnotations(); - // copy over the annotations as bytes - for (Map.Entry annotation : annotations.entrySet()) { - span.addKVAnnotation(StringUtil.toBytes(annotation.getKey()), StringUtil.toBytes(annotation.getValue())); - } + if (span == null) { + return; + } + Map annotations = conn.getCustomTracingAnnotations(); + // copy over the annotations as bytes + for (Map.Entry annotation : annotations.entrySet()) { + span.addKVAnnotation(toBytes(annotation.getKey()), toBytes(annotation.getValue())); } } diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/JDBCUtil.java b/phoenix-core/src/main/java/org/apache/phoenix/util/JDBCUtil.java index db81cf1a2b4..4e9f01b9151 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/util/JDBCUtil.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/util/JDBCUtil.java @@ -18,7 +18,7 @@ package org.apache.phoenix.util; import static com.google.common.collect.Maps.newHashMapWithExpectedSize; -import static org.apache.phoenix.query.QueryServices.TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX; +import static org.apache.phoenix.util.PhoenixRuntime.ANNOTATION_ATTRIB_PREFIX; import java.sql.SQLException; import java.util.Map; @@ -69,6 +69,9 @@ public static String findProperty(String url, Properties info, String propName) return propValue; } + /** + * Returns a map that contains connection properties from both info and url. + */ private static Map getCombinedConnectionProperties(String url, Properties info) { Map result = newHashMapWithExpectedSize(info.size()); for (String propName : info.stringPropertyNames()) { @@ -87,16 +90,16 @@ private static Map getCombinedConnectionProperties(String url, P return result; } - public static Map getCustomTracingAnnotations(@NotNull String url, @NotNull Properties info) { + public static Map getAnnotations(@NotNull String url, @NotNull Properties info) { Preconditions.checkNotNull(url); Preconditions.checkNotNull(info); Map combinedProperties = getCombinedConnectionProperties(url, info); Map result = newHashMapWithExpectedSize(combinedProperties.size()); for (Map.Entry prop : combinedProperties.entrySet()) { - if (prop.getKey().startsWith(TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX) && - prop.getKey().length() > TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX.length()) { - result.put(prop.getKey().substring(TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX.length()), prop.getValue()); + if (prop.getKey().startsWith(ANNOTATION_ATTRIB_PREFIX) && + prop.getKey().length() > ANNOTATION_ATTRIB_PREFIX.length()) { + result.put(prop.getKey().substring(ANNOTATION_ATTRIB_PREFIX.length()), prop.getValue()); } } return result; diff --git a/phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java b/phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java index af4c9e0d55c..7de27c24db9 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java @@ -105,6 +105,13 @@ public class PhoenixRuntime { * configuration properties */ public static final String TENANT_ID_ATTRIB = "TenantId"; + + /** + * Use this connection property prefix for annotations that you want to show up in traces and log lines emitted by Phoenix. + * This is useful for annotating connections with information available on the client (e.g. user or session identifier) and + * having these annotation automatically passed into log lines and traces by Phoenix. + */ + public static final String ANNOTATION_ATTRIB_PREFIX = "phoenix.annotation."; /** * Use this as the zookeeper quorum name to have a connection-less connection. This enables diff --git a/phoenix-core/src/test/java/org/apache/phoenix/util/JDBCUtilTest.java b/phoenix-core/src/test/java/org/apache/phoenix/util/JDBCUtilTest.java index 5783bfb6ed2..e138806a28b 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/util/JDBCUtilTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/util/JDBCUtilTest.java @@ -17,7 +17,7 @@ */ package org.apache.phoenix.util; -import static org.apache.phoenix.query.QueryServices.TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX; +import static org.apache.phoenix.util.PhoenixRuntime.ANNOTATION_ATTRIB_PREFIX; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; @@ -31,7 +31,7 @@ public class JDBCUtilTest { @Test public void testGetCustomTracingAnnotationsWithNone() { String url = "localhost;TenantId=abc;"; - Map customAnnotations = JDBCUtil.getCustomTracingAnnotations(url, new Properties()); + Map customAnnotations = JDBCUtil.getAnnotations(url, new Properties()); assertTrue(customAnnotations.isEmpty()); } @@ -44,13 +44,13 @@ public void testGetCustomTracingAnnotationInBothPropertiesAndURL() { String annotKey3 = "key3"; String annotVal3 = "val3"; - String url= "localhost;" + TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX + annotKey1 + '=' + annotVal1; + String url= "localhost;" + ANNOTATION_ATTRIB_PREFIX + annotKey1 + '=' + annotVal1; Properties prop = new Properties(); - prop.put(TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX + annotKey2, annotVal2); - prop.put(TRACING_CUSTOM_ANNOTATION_ATTRIB_PREFIX + annotKey3, annotVal3); + prop.put(ANNOTATION_ATTRIB_PREFIX + annotKey2, annotVal2); + prop.put(ANNOTATION_ATTRIB_PREFIX + annotKey3, annotVal3); - Map customAnnotations = JDBCUtil.getCustomTracingAnnotations(url, prop); + Map customAnnotations = JDBCUtil.getAnnotations(url, prop); assertEquals(3, customAnnotations.size()); assertEquals(annotVal1, customAnnotations.get(annotKey1)); assertEquals(annotVal2, customAnnotations.get(annotKey2)); diff --git a/phoenix-hadoop-compat/src/main/java/org/apache/phoenix/trace/TracingCompat.java b/phoenix-hadoop-compat/src/main/java/org/apache/phoenix/trace/TracingCompat.java index e0a34106a1e..032e38a1b85 100644 --- a/phoenix-hadoop-compat/src/main/java/org/apache/phoenix/trace/TracingCompat.java +++ b/phoenix-hadoop-compat/src/main/java/org/apache/phoenix/trace/TracingCompat.java @@ -86,4 +86,4 @@ public static final String getTraceMetricName(long traceId) { public static final String getTraceMetricName(String traceId) { return METRIC_SOURCE_KEY + traceId; } -} +} \ No newline at end of file