PHOENIX-5215 Migrate from HTrace to OpenTelemetry tracing - #2586
PHOENIX-5215 Migrate from HTrace to OpenTelemetry tracing#2586lfrancke wants to merge 14 commits into
Conversation
…jars Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…HBase Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
48dac8e to
584fc84
Compare
|
Yetus failed twice. The first one was real and I fixed the issues but the second one is no. I believe those test failures are unrelated and seem to happen on master as well. |
There was a problem hiding this comment.
Pull request overview
Replaces Phoenix’s obsolete HTrace integration with an OpenTelemetry facade and updates tracing call sites, packaging, configuration, and tests.
Changes:
- Removes HTrace dependencies, sinks, readers, utilities, and tests.
- Adds
PhoenixTracingand migrates client/server instrumentation to OpenTelemetry. - Updates shaded artifacts, runtime configuration, and tracing integration tests.
Reviewed changes
Copilot reviewed 48 out of 50 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/config/checkstyle/checker.xml |
Changes static-import ordering. |
pom.xml |
Manages OpenTelemetry and bans HTrace. |
phoenix-server/pom.xml |
Prevents OpenTelemetry shading. |
phoenix-mapreduce-byo-shaded-hbase/pom.xml |
Removes HTrace shading exception. |
phoenix-core/src/test/java/org/apache/phoenix/trace/TraceSpanReceiverTest.java |
Removes receiver tests. |
phoenix-core/src/test/java/org/apache/phoenix/metrics/LoggingSink.java |
Removes tracing utility dependency. |
phoenix-core/src/test/java/org/apache/phoenix/mapreduce/util/PhoenixMapReduceUtilTest.java |
Removes HTrace jar assertion. |
phoenix-core/src/test/java/org/apache/hadoop/metrics2/lib/ExposedMetricsInfoImpl.java |
Removes obsolete test helper. |
phoenix-core/src/test/java/org/apache/hadoop/metrics2/impl/ExposedMetricsRecordImpl.java |
Removes obsolete test helper. |
phoenix-core/src/test/java/org/apache/hadoop/metrics2/impl/ExposedMetricCounterLong.java |
Removes obsolete test helper. |
phoenix-core/src/it/java/org/apache/phoenix/trace/TracingTestUtil.java |
Removes legacy sink helper. |
phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingIT.java |
Tests no-SDK and TRACE behavior. |
phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTracingEndToEndIT.java |
Removes legacy tracing ITs. |
phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTagImpl.java |
Removes legacy metrics helper. |
phoenix-core/src/it/java/org/apache/phoenix/trace/PhoenixTableMetricsWriterIT.java |
Removes table-writer ITs. |
phoenix-core/src/it/java/org/apache/phoenix/trace/BaseTracingTestIT.java |
Removes HTrace test fixture. |
phoenix-core/pom.xml |
Replaces test HTrace dependency. |
phoenix-core-server/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixMapReduceUtil.java |
Stops shipping HTrace jars. |
phoenix-core-server/src/main/java/org/apache/phoenix/index/PhoenixTransactionalIndexer.java |
Migrates transactional index spans. |
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/LockManager.java |
Migrates row-lock tracing. |
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java |
Migrates index observer spans. |
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/Indexer.java |
Migrates indexer spans. |
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java |
Removes HTrace initialization. |
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java |
Migrates scanner span lifecycle. |
phoenix-core-server/pom.xml |
Adds provided OpenTelemetry APIs. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/util/Tracing.java |
Removes HTrace facade. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/util/NullSpan.java |
Removes HTrace no-op span. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/util/ConfigurationAdapter.java |
Removes sampler configuration adapter. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/TracingUtils.java |
Removes legacy tracing utilities. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/TracingIterator.java |
Manages OpenTelemetry iterator spans. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/TraceWriter.java |
Removes trace-table writer. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/TraceSpanReceiver.java |
Removes HTrace receiver. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/TraceReader.java |
Removes trace-table reader. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/PhoenixTracing.java |
Adds the OpenTelemetry facade. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/PhoenixMetricsSink.java |
Removes legacy metrics sink. |
phoenix-core-client/src/main/java/org/apache/phoenix/trace/MetricsInfoImpl.java |
Removes tracing metrics helper. |
phoenix-core-client/src/main/java/org/apache/phoenix/query/QueryServicesOptions.java |
Decouples default frequency from HTrace. |
phoenix-core-client/src/main/java/org/apache/phoenix/query/QueryServices.java |
Deprecates legacy tracing properties. |
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java |
Traces statement execution with OpenTelemetry. |
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java |
Migrates connection tracing state. |
phoenix-core-client/src/main/java/org/apache/phoenix/iterate/SerialIterators.java |
Propagates context to serial scans. |
phoenix-core-client/src/main/java/org/apache/phoenix/iterate/ParallelIterators.java |
Propagates context to parallel scans. |
phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java |
Migrates mutation spans. |
phoenix-core-client/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java |
Adds query execution spans. |
phoenix-core-client/src/main/java/org/apache/phoenix/compile/TraceQueryPlan.java |
Adapts TRACE statements to OpenTelemetry. |
phoenix-core-client/src/main/antlr3/PhoenixSQL.g |
Uses the new TRACE option parser. |
phoenix-core-client/pom.xml |
Adds provided OpenTelemetry dependencies. |
phoenix-client-parent/pom.xml |
Prevents client-side OpenTelemetry shading. |
bin/hadoop-metrics2-phoenix.properties |
Retires Phoenix trace sink configuration. |
bin/hadoop-metrics2-hbase.properties |
Retires HBase trace sink configuration. |
Suppressed comments (1)
phoenix-core-client/src/main/java/org/apache/phoenix/trace/PhoenixTracing.java:214
- If
action.get()throws synchronously, the exception escapes without recording the error or ending the newly created span. This leaves a live span that can never be exported. Handle synchronousRuntimeException/Errorthe same way as exceptional future completion.
try (Scope ignored = span.makeCurrent()) {
CompletableFuture<T> future = action.get();
endSpan(future, span);
return future;
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <bannedDependencies> | ||
| <message>HTrace is retired, use OpenTelemetry via PhoenixTracing</message> | ||
| <searchTransitive>true</searchTransitive> | ||
| <excludes> | ||
| <exclude>org.apache.htrace:*</exclude> | ||
| </excludes> |
There was a problem hiding this comment.
Yep, that was my fault. I did not realize that Hadoop 3.2.4 still has HTrace. I've now changed the rule to only look for direct dependencies we add ourselves.
| Span child = | ||
| Tracing.child(span, "Writing mutation batch for table: " + Bytes.toString(htableName)); | ||
| PhoenixTracing.createSpan("phoenix.mutation.batch.write." + Bytes.toString(htableName)); |
There was a problem hiding this comment.
Yup, good catch. This should be fixed now.
We didn't use the suggested finally because there is already a finally which runs on every retry iteration. Ending it there would kill the just created span immediately.
| } 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) { |
There was a problem hiding this comment.
This was a decision I (human me, not AI) made. I don't know when/how this happened but anyway the existing code fails SpotBugs - maybe the rule was introduced after the code already existed? I don't know. But SpotBugs suggests exactly this pattern as a fix.
I can revert it but then we'll need a SpotBugs exclusion for that line. Personally I think it makes sense to keep this in but I'll let you decide.
| private static Span createSpan(String name, SpanKind kind) { | ||
| return getTracer().spanBuilder(name).setSpanKind(kind).startSpan(); | ||
| } |
There was a problem hiding this comment.
The docs were wrong, that's true. But other than that this is a port from the HBase version.
Server side this makes no difference as HBase already creates a span, but client side it runs wherever and in those cases it does make sense to start something.
We've chosen to fix the javadoc instead of changing the behavior.
Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Generated-by: Claude Opus 5 (Claude Code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
584fc84 to
cb3982a
Compare
|
@apurtell Thanks for kicking of the review. I've addressed all comments but can't kick off a re-review myself. |
What changes were proposed in this pull request?
Important
This builds upon #2394 which has stalled for four months now. The four commits from @xavifeds8 are taken as is and later commits build on top of those four.
Removes Apache HTrace from Phoenix and replaces it with a thin OpenTelemetry facade,
PhoenixTracing, adapted from HBase'sTraceUtil(HBASE-22120).Per the previous discussions on the mailing list and issue this doesn't depend on an opentelemetry SDK itself and instead relies on the one provided by HBase (2.5+).
HTrace as a direct dependency is banned going forward by maven-enforcer (we still get plenty of transitive ones in the 2.5 profile)
References
Yes, this has been a long time coming :)
Changes on top of #2394
Rebase onto master:
PhoenixMapReduceUtil.addPhoenixDependencyJars, added to master after PHOENIX-5215 Migrate from HTrace to OpenTelemetry tracing #2394 was written, still shippedhtrace-coreto YARN containers.htrace-core4transitively, not much we can do about thatCorrectness:
opentelemetry.versionto 1.15.0. PHOENIX-5215 Migrate from HTrace to OpenTelemetry tracing #2394 used 1.49.0, but HBase 2.5.3, 2.5.10 and 2.6.1 ship 1.15.0 (2.5.14 and 2.6.3+ ship 1.49.0). Depending on the older version should be safer.ScopeonPhoenixConnectionandTracingIterator. AScoperestores a thread-local and must be closed on the thread that opened it, but aConnectionis closed by an arbitrary thread (including the HA framework) and an iterator by whichever thread finishes with it.TraceUtilproperly in thePhoenixTracingjavadoc, and fixed javadoc placement on the deprecated tracing properties, which sat after@Deprecatedand so was not javadoc at all.Why are the changes needed?
Phoenix's tracing has been non-functional for years, so this removes a dead dependency (with vulnerabilities) and paves the way for a more modern OpenTelemetry pipeline integrated into the stuff HBase provides
Does this PR introduce any user-facing change?
Yes, though only to a feature that was already non-functional anyway (see above).
Removed: writing traces to an HBase table.
PhoenixMetricsSinkandTraceWriterare gone, soSYSTEM.TRACING_STATSis no longer written. The sink configuration inbin/hadoop-metrics2-phoenix.propertiesandbin/hadoop-metrics2-hbase.propertiesis retained but commented out, with a note pointing at the OpenTelemetry agent.SYSTEM.TRACING_STATSitself, the tracing webapp andbin/traceserver.pyare untouched, that is the scope of #1721.Changed:
TRACE ONis now inert. The SQL grammar is untouched and the hook remains, soTRACE ONandTRACE OFFstill parse and execute without error, but no Phoenix-side trace is started. Previously Phoenix owned a sampler and started its own root traces; under OpenTelemetry, sampling belongs to the SDK and the root span comes from whatever instrumented the caller. I decided to keep this PR to the removal of HTrace and replacing it with OTel. MakingTRACE ONuseful again can come in a follow-up.Deprecated: Phoenix tracing properties.
phoenix.trace.frequency,phoenix.trace.probability.threshold,phoenix.trace.enabled,phoenix.trace.batchSize,phoenix.trace.threadPoolSize,phoenix.trace.traceBufferSizeandphoenix.trace.read.pagesizeare marked@Deprecatedand no longer read. I'd also be happy to just remove them. Opinions welcome. OpenTelemetry is configured through the agent, for exampleOTEL_EXPORTER_OTLP_ENDPOINT.Unchanged: everything else. With no agent attached there is no behaviour change and no overhead.
How was this patch tested?
New
PhoenixTracingITcovers the no-op path with no SDK present,TRACE ON/TRACE OFFround-tripping, and that no thread is left pinned to a staleContextafter the connection closes.Locally, against the HBase 2.6 profile:
PhoenixTracingIT(3 tests) greenphoenix-pherfincluding its integration tests green.mvn spotless:checkclean across all modules.Packaging, verified by inspecting the built artifacts.
Was this patch authored or co-authored using generative AI tooling?
Yes. Partially
Generated-by: Claude Opus 5 (Claude Code)
The four commits by @xavifeds8 are preserved unchanged from #2394, no idea about those. The commits above them carry
Generated-by:andCo-authored-by:trailers individually, but even those are partially human authored. I've attempted this whole exercise already two or so years ago....and took some of what I had back then.Per the ASF Generative Tooling Guidance:
PhoenixTracingis adapted from HBase'sTraceUtil, which is Apache-2.0 and therefore compatible, and this derivation is stated in the class javadoc.