Copilot commented on code in PR #2586:
URL: https://github.com/apache/phoenix/pull/2586#discussion_r3789336395
##########
phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java:
##########
@@ -1503,7 +1511,7 @@ private void sendMutations(Iterator<Entry<TableInfo,
List<Mutation>>> mutationsI
// create a span per target table
// TODO maybe we can be smarter about the table name to string here?
Span child =
- Tracing.child(span, "Writing mutation batch for table: " +
Bytes.toString(htableName));
+ PhoenixTracing.createSpan("phoenix.mutation.batch.write." +
Bytes.toString(htableName));
Review Comment:
This per-table span is ended only on a successful batch or the
metadata-retry branch. Any final checked failure (the `sqlE` thrown at line
1799) or rethrown runtime failure exits `sendMutations` without ending it, so
failed mutation spans remain live and are never exported. Wrap the complete
per-table/retry lifecycle in a `finally` that records the failure and ends
whichever child span is active.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/trace/PhoenixTracing.java:
##########
@@ -0,0 +1,385 @@
+/*
+ * 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.trace;
+
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.Supplier;
+import org.apache.phoenix.call.CallWrapper;
+import org.apache.phoenix.job.JobManager.JobCallable;
+import org.apache.phoenix.monitoring.TaskExecutionMetricsHolder;
+
+import org.apache.phoenix.thirdparty.com.google.common.base.Preconditions;
+
+/**
+ * Central tracing facade for Apache Phoenix using OpenTelemetry. All methods
are no-ops with zero
+ * overhead when no OpenTelemetry SDK is configured.
+ * <p>
+ * Adapted from Apache HBase's {@code org.apache.hadoop.hbase.trace.TraceUtil}
(HBASE-22120). The
+ * span helpers, the future and runnable wrappers, and the throwing functional
interfaces are taken
+ * from it largely verbatim.
+ * @see <a
href="https://issues.apache.org/jira/browse/PHOENIX-5215";>PHOENIX-5215</a>
+ */
+public final class PhoenixTracing {
+
+ private static final String INSTRUMENTATION_NAME = "org.apache.phoenix";
+
+ private PhoenixTracing() {
+ }
+
+ /**
+ * Returns the global tracer for Phoenix. The tracer is obtained from {@link
GlobalOpenTelemetry}
+ * on each call (the OTel SDK caches it internally, so there is no
performance penalty). This
+ * avoids issues with eager initialization when the SDK has not been
configured yet.
+ */
+ public static Tracer getTracer() {
+ return GlobalOpenTelemetry.getTracer(INSTRUMENTATION_NAME);
+ }
+
+ /**
+ * Create a {@link SpanKind#INTERNAL} span. This is the default for most
Phoenix operations (query
+ * compilation, mutation processing, index maintenance, etc.).
+ */
+ public static Span createSpan(String name) {
+ return createSpan(name, SpanKind.INTERNAL);
+ }
+
+ /**
+ * Create a span with the given {@code kind}. Note that OpenTelemetry
expects at most one
+ * {@link SpanKind#CLIENT} span and one {@link SpanKind#SERVER} span per
traced request, so use
+ * this with caution for kinds other than {@link SpanKind#INTERNAL}.
+ */
+ private static Span createSpan(String name, SpanKind kind) {
+ return getTracer().spanBuilder(name).setSpanKind(kind).startSpan();
+ }
Review Comment:
This unconditionally starts a root span when an SDK is installed but no span
is current. That contradicts this class's `isRecording()` contract and the PR's
stated model that Phoenix only adds children to a trace started by an
instrumented caller; unguarded call sites such as statement execution and index
maintenance will otherwise create Phoenix-owned traces. Return an invalid span
unless the current span is recording.
This issue also appears on line 210 of the same file.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java:
##########
@@ -1660,6 +1668,12 @@ private IOException
updateTableRegionCacheIfNecessary(IOException ioe) {
PhoenixKeyValueUtil.getEstimatedRowMutationSizeWithBatch(this.mutationsMap);
}
areAllBatchesSuccessful = true;
+ } catch (RuntimeException e) {
+ // A RuntimeException here is a Phoenix bug, not a failed mutation.
Rethrow it rather
+ // than wrapping it in a CommitException, which would report it as a
server-side commit
+ // failure. Checked exceptions from hTable.batch() still fall
through to the handler
+ // below. This is the pattern SpotBugs REC_CATCH_EXCEPTION asks for.
+ throw e;
} catch (Exception e) {
Review Comment:
This changes non-tracing behavior contrary to the PR's stated scope: runtime
failures from mutation processing were previously handled by the following
`catch (Exception)` and surfaced as `CommitException`, but now escape directly.
That changes the public failure contract for commits and should not be bundled
into the tracing migration; preserve the existing wrapping behavior (or isolate
and document this behavioral change separately).
##########
pom.xml:
##########
@@ -1799,6 +1794,31 @@
<bannedImport>com.sun.istack.**</bannedImport>
</bannedImports>
</RestrictImports>
+ <RestrictImports>
+ <includeTestCode>true</includeTestCode>
+ <reason>HTrace is removed, use OpenTelemetry via
PhoenixTracing</reason>
+ <bannedImports>
+ <bannedImport>org.apache.htrace.**</bannedImport>
+ </bannedImports>
+ </RestrictImports>
+ </rules>
+ </configuration>
+ </execution>
+ <execution>
+ <id>banned-illegal-dependencies</id>
+ <goals>
+ <goal>enforce</goal>
+ </goals>
+ <phase>validate</phase>
+ <configuration>
+ <rules>
+ <bannedDependencies>
+ <message>HTrace is retired, use OpenTelemetry via
PhoenixTracing</message>
+ <searchTransitive>true</searchTransitive>
+ <excludes>
+ <exclude>org.apache.htrace:*</exclude>
+ </excludes>
Review Comment:
The new transitive ban breaks the supported `phoenix-hbase-compat-2.5.0`
profile. That profile selects Hadoop 3.2.4 (`pom.xml:2260`), and Hadoop 3.2.4's
`hadoop-common` POM declares `org.apache.htrace:htrace-core4`; removing the
three existing exclusions means the enforcer now rejects the profile during
validation. Keep the exclusions for dependencies used by this profile (or
otherwise exempt that unavoidable transitive dependency).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
