Copilot commented on code in PR #10477:
URL: https://github.com/apache/ozone/pull/10477#discussion_r3432516857
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -54,6 +56,14 @@ public final class TracingUtil {
private static volatile boolean isInit = false;
private static Tracer tracer = OpenTelemetry.noop().getTracer("noop");
private static SdkTracerProvider sdkTracerProvider;
+ private static volatile Mode mode = Mode.NOOP;
+
+ /* MODES of tracing are as follows:
+ NOOP = off, OZONE = Ozone owns tracing, APPLICATION = join the app's trace,
+ INCOMING_ONLY = server exports spans only when a client sends trace context.
+ */
Review Comment:
This block comment isn’t formatted consistently with the rest of the file
(missing leading `*` on lines, awkward wrapping). Converting it to Javadoc (or
`//` lines) improves readability and avoids potential style/checkstyle issues.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -275,7 +383,10 @@ public static <R, E extends Exception> R
executeInNewSpan(String spanName,
* @return the value returned by {@code supplier}
*/
private static <R, E extends Exception> R executeInSpan(Span span,
- CheckedSupplier<R, E> supplier) throws E {
+ CheckedSupplier<R,
E> supplier) throws E {
Review Comment:
The wrapped parameter line is indented far beyond the normal continuation
indent used elsewhere in this file (e.g., `initTracing(...)` at lines 74-75).
Please format using the standard continuation indentation to keep the file
consistent and avoid formatter/checkstyle churn.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -307,7 +422,7 @@ private static <E extends Exception> void
executeInSpan(Span span,
* Execute a new function as a child span of the parent.
*/
public static <E extends Exception> void executeAsChildSpan(String spanName,
- String parentName, CheckedRunnable<E> runnable) throws E {
+ String
parentName, CheckedRunnable<E> runnable) throws E {
Review Comment:
The wrapped parameter line is indented far beyond the normal continuation
indent used elsewhere in this file (e.g., `initTracing(...)` at lines 74-75).
Please format using the standard continuation indentation to keep the file
consistent and avoid formatter/checkstyle churn.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -264,7 +372,7 @@ public static <E extends Exception> void
executeInNewSpan(String spanName,
* Execute {@code supplier} inside an activated new span.
*/
public static <R, E extends Exception> R executeInNewSpan(String spanName,
- CheckedSupplier<R, E> supplier) throws E {
+ CheckedSupplier<R,
E> supplier) throws E {
Review Comment:
The wrapped parameter line is indented far beyond the normal continuation
indent used elsewhere in this file (e.g., `initTracing(...)` at lines 74-75).
Please format using the standard continuation indentation to keep the file
consistent and avoid formatter/checkstyle churn.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -176,6 +281,9 @@ public static String exportCurrentSpan() {
*/
public static Span importAndCreateSpan(String name, String encodedParent) {
if (encodedParent == null || encodedParent.isEmpty()) {
+ if (mode == Mode.APPLICATION || mode == Mode.INCOMING_ONLY) {
+ return Span.getInvalid();
+ }
return tracer.spanBuilder(name).setNoParent().startSpan();
}
Review Comment:
The new guard correctly avoids creating root spans for `null`/empty parents
in APPLICATION/INCOMING_ONLY modes, but a **non-empty yet invalid/malformed**
W3C carrier can still end up producing a new root span: `extract(...)` may
yield a context with no valid span, and `spanBuilder(...).setParent(extract)`
will then behave like a root span.
To preserve the “only continue existing traces” contract for these modes,
consider also checking the extracted context’s span validity and returning
`Span.getInvalid()` when it’s not valid.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -255,7 +363,7 @@ static Map<String, LoopSampler>
parseSpanSamplingConfig(String configStr) {
* If a parent span exists in the current context, this becomes a child span.
*/
public static <E extends Exception> void executeInNewSpan(String spanName,
- CheckedRunnable<E> runnable) throws E {
+ CheckedRunnable<E>
runnable) throws E {
Review Comment:
The wrapped parameter line is indented far beyond the normal continuation
indent used elsewhere in this file (e.g., `initTracing(...)` at lines 74-75).
Please format using the standard continuation indentation to keep the file
consistent and avoid formatter/checkstyle churn.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -291,7 +402,11 @@ private static <R, E extends Exception> R
executeInSpan(Span span,
* Execute {@code runnable} in the given {@code span}.
*/
private static <E extends Exception> void executeInSpan(Span span,
- CheckedRunnable<E> runnable) throws E {
+ CheckedRunnable<E>
runnable) throws E {
Review Comment:
The wrapped parameter line is indented far beyond the normal continuation
indent used elsewhere in this file (e.g., `initTracing(...)` at lines 74-75).
Please format using the standard continuation indentation to keep the file
consistent and avoid formatter/checkstyle churn.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -409,7 +533,7 @@ public String get(Function<String, String> carrier, String
key) {
public static TraceCloseable createActivatedSpanFromW3cHttpHeaders(
String spanName, Function<String, String> getHeader, ConfigurationSource
conf) {
- if (conf == null || !isTracingEnabled(conf)) {
+ if (conf == null || !(isTracingActive() || shouldInstallTraceProxy(conf)))
{
Review Comment:
This condition uses a double-negative (`!(A || B)`) which is harder to scan
and review. Using De Morgan’s law makes the intent clearer without changing
behavior.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
