You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2021/08/18 14:11:02 UTC
[GitHub] [phoenix] richardantal commented on a change in pull request #1282: PHOENIX-5215 opentelemetry changes replacing htrace - WIP
richardantal commented on a change in pull request #1282:
URL: https://github.com/apache/phoenix/pull/1282#discussion_r691057017
##########
File path: phoenix-core/pom.xml
##########
@@ -444,8 +455,8 @@
<artifactId>protobuf-java</artifactId>
</dependency>
<dependency>
- <groupId>org.apache.htrace</groupId>
- <artifactId>htrace-core</artifactId>
+ <groupId>io.opentelemetry</groupId>
+ <artifactId>opentelemetry-api</artifactId>
Review comment:
Already defined in line 404
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java
##########
@@ -371,11 +372,11 @@ public final ResultIterator iterator(final Map<ImmutableBytesPtr,ServerCache> ca
}
// wrap the iterator so we start/end tracing as we expect
- if (Tracing.isTracing()) {
- TraceScope scope = Tracing.startNewSpan(context.getConnection(),
- "Creating basic query for " + getPlanSteps(iterator));
- if (scope.getSpan() != null) return new TracingIterator(scope, iterator);
- }
+
+ Span span = TraceUtil.getGlobalTracer().spanBuilder("Creating basic query for " + getPlanSteps(iterator)).startSpan();
+
+ if (span.isRecording()) return new TracingIterator(span, iterator);
Review comment:
Is it going to be true anytime?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/trace/TracingIterator.java
##########
@@ -19,45 +19,48 @@
import java.sql.SQLException;
+import io.opentelemetry.api.trace.Span;
import org.apache.phoenix.iterate.DelegateResultIterator;
import org.apache.phoenix.iterate.ResultIterator;
import org.apache.phoenix.schema.tuple.Tuple;
-import org.apache.htrace.TraceScope;
+
Review comment:
nit: extra line
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java
##########
@@ -17,41 +17,23 @@
*/
package org.apache.phoenix.trace.util;
-import static org.apache.phoenix.util.StringUtil.toBytes;
-
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Properties;
import java.util.concurrent.Callable;
-import javax.annotation.Nullable;
-
-import org.apache.hadoop.conf.Configuration;
-import org.apache.htrace.HTraceConfiguration;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.context.Scope;
Review comment:
i think it can be deleted, as it is not used.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java
##########
@@ -201,6 +201,7 @@
import org.apache.phoenix.schema.types.PDataType;
import org.apache.phoenix.schema.types.PLong;
import org.apache.phoenix.schema.types.PVarchar;
+import org.apache.phoenix.trace.TraceUtil;
Review comment:
nit: not used
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTableMetricsWriterIT.java
##########
@@ -84,8 +83,8 @@ public void writeMetrics() throws Exception {
Span span = createNewSpan(traceid, parentid, spanid, description, startTime, endTime,
processid, annotation);
- Tracer.getInstance().deliver(span);
- assertTrue("Span never committed to table", latch.await(30, TimeUnit.SECONDS));
+ //Tracer.getInstance().deliver(span);
Review comment:
These lines should be deleted
##########
File path: phoenix-core/pom.xml
##########
@@ -398,6 +398,17 @@
<type>test-jar</type>
</dependency>
+ <!-- Tracing Dependencies -->
+ <dependency>
+ <groupId>io.opentelemetry</groupId>
+ <artifactId>opentelemetry-api</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>io.opentelemetry</groupId>
+ <artifactId>opentelemetry-context</artifactId>
+ <version>${opentelemetry.version}</version>
Review comment:
version should be in the dependencyManagement
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixDriver.java
##########
@@ -60,7 +60,7 @@
*
* @since 0.1
*/
-public final class PhoenixDriver extends PhoenixEmbeddedDriver {
+public final class PhoenixDriver extends PhoenixEmbeddedDriver {
Review comment:
nit: extra space
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/trace/TraceWriter.java
##########
@@ -264,7 +256,7 @@ protected Connection getConnection(String tableName) {
try {
// create the phoenix connection
Properties props = new Properties();
- props.setProperty(QueryServices.TRACING_FREQ_ATTRIB, Tracing.Frequency.NEVER.getKey());
+ //props.setProperty(QueryServices.TRACING_FREQ_ATTRIB, Tracing.Frequency.NEVER.getKey());
Review comment:
delete this line
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/trace/util/Tracing.java
##########
@@ -17,41 +17,23 @@
*/
package org.apache.phoenix.trace.util;
-import static org.apache.phoenix.util.StringUtil.toBytes;
-
-import java.util.HashMap;
-import java.util.Map;
-import java.util.Properties;
import java.util.concurrent.Callable;
-import javax.annotation.Nullable;
-
-import org.apache.hadoop.conf.Configuration;
-import org.apache.htrace.HTraceConfiguration;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.context.Scope;
import org.apache.phoenix.call.CallRunner;
import org.apache.phoenix.call.CallWrapper;
import org.apache.phoenix.jdbc.PhoenixConnection;
-import org.apache.phoenix.parse.TraceStatement;
-import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.job.JobManager;
Review comment:
not used
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org