sumitagrawl commented on code in PR #10477:
URL: https://github.com/apache/ozone/pull/10477#discussion_r3588750952
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -197,11 +267,6 @@ private static void initialize(String serviceName,
TracingConfig tracingConfig)
* @return encoded tracing context.
*/
public static String exportCurrentSpan() {
- Span currentSpan = Span.current();
Review Comment:
if span is not valid, what is the content of exportCurrentSpan() to check
impact?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -451,14 +534,17 @@ 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)) {
Review Comment:
isTracingEnabled() can be removed
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -65,14 +70,20 @@ private TracingUtil() {
*/
public static synchronized void initTracing(
String serviceName, TracingConfig tracingConfig) {
- if (!tracingConfig.isTracingEnabled() || isInit) {
+ initTracing(serviceName, tracingConfig, false);
+ }
+
+ private static synchronized void initTracing(
+ String serviceName, TracingConfig tracingConfig, boolean preferOzone) {
Review Comment:
can change preferOzone to isReconfig
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -129,23 +140,90 @@ public static <R, E extends Exception> R execute(
}
}
- private static void shutdownTracing() {
- if (sdkTracerProvider == null) {
- return;
- }
+ static void shutdownTracing() {
try {
- sdkTracerProvider.shutdown().join(10L, TimeUnit.SECONDS);
+ if (sdkTracerProvider != null) {
+ sdkTracerProvider.shutdown().join(10L, TimeUnit.SECONDS);
+ }
} catch (Exception e) {
LOG.warn("Tracing shutdown failed", e);
} finally {
sdkTracerProvider = null;
batchSpanProcessor = null;
tracer = OpenTelemetry.noop().getTracer("noop");
+ tracingEnabled = false;
+ applicationAware = false;
isInit = false;
}
}
- private static void initialize(String serviceName, TracingConfig
tracingConfig) {
+ private static void initialize(String serviceName, TracingConfig cfg,
boolean preferOzone) {
+ tracingEnabled = cfg.isTracingEnabled();
+ applicationAware = cfg.isApplicationAware();
+
+ if (!tracingEnabled && !applicationAware) {
+ tracer = OpenTelemetry.noop().getTracer(GLOBAL_TRACER_NAME);
+ return;
+ }
+
+ // Server reconfiguration reprioritizes Ozone's SDK over any adopted
global,
+ // and re-registers the global name and tracer.
+ if (preferOzone && tracingEnabled) {
+ initOzoneSdk(serviceName, cfg, true);
+ return;
+ }
+
+ // Global first: adopt an application-registered GlobalOpenTelemetry when
present.
+ if (GlobalOpenTelemetry.isSet() &&
isRealGlobal(GlobalOpenTelemetry.get())) {
+ tracer = GlobalOpenTelemetry.get().getTracer(GLOBAL_TRACER_NAME);
+ LOG.info("Tracing: adopted application GlobalOpenTelemetry");
+ return;
+ }
+
+ initOzoneSdk(serviceName, cfg, tracingEnabled);
Review Comment:
we can set always if trace is valid
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -197,11 +267,6 @@ private static void initialize(String serviceName,
TracingConfig tracingConfig)
* @return encoded tracing context.
*/
public static String exportCurrentSpan() {
- Span currentSpan = Span.current();
Review Comment:
can check what is the content is span is not enabled.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -218,11 +283,21 @@ public static String exportCurrentSpan() {
*/
public static Span importAndCreateSpan(String name, String encodedParent) {
if (encodedParent == null || encodedParent.isEmpty()) {
+ if (!canStartSpanWithoutParent()) {
Review Comment:
check for sdk null or nops, to see if can import and use
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -254,6 +329,11 @@ public static boolean isTracingEnabled(ConfigurationSource
conf) {
return conf.getObject(TracingConfig.class).isTracingEnabled();
}
+ public static boolean isTracingActive(ConfigurationSource conf) {
+ TracingConfig tc = conf.getObject(TracingConfig.class);
+ return tc.isTracingEnabled() || tc.isApplicationAware();
Review Comment:
applicationAware && SDKConfigured
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -218,11 +283,21 @@ public static String exportCurrentSpan() {
*/
public static Span importAndCreateSpan(String name, String encodedParent) {
if (encodedParent == null || encodedParent.isEmpty()) {
+ if (!canStartSpanWithoutParent()) {
+ return Span.getInvalid();
+ }
return tracer.spanBuilder(name).setNoParent().startSpan();
}
W3CTraceContextPropagator propagator =
W3CTraceContextPropagator.getInstance();
Context extract = propagator.extract(Context.current(), encodedParent, new
TextExtractor());
+ Span extractedParent = Span.fromContext(extract);
Review Comment:
this code can be removed
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/tracing/TracingUtil.java:
##########
@@ -129,23 +140,90 @@ public static <R, E extends Exception> R execute(
}
}
- private static void shutdownTracing() {
- if (sdkTracerProvider == null) {
- return;
- }
+ static void shutdownTracing() {
try {
- sdkTracerProvider.shutdown().join(10L, TimeUnit.SECONDS);
+ if (sdkTracerProvider != null) {
+ sdkTracerProvider.shutdown().join(10L, TimeUnit.SECONDS);
+ }
} catch (Exception e) {
LOG.warn("Tracing shutdown failed", e);
} finally {
sdkTracerProvider = null;
batchSpanProcessor = null;
tracer = OpenTelemetry.noop().getTracer("noop");
+ tracingEnabled = false;
+ applicationAware = false;
isInit = false;
}
}
- private static void initialize(String serviceName, TracingConfig
tracingConfig) {
+ private static void initialize(String serviceName, TracingConfig cfg,
boolean preferOzone) {
+ tracingEnabled = cfg.isTracingEnabled();
+ applicationAware = cfg.isApplicationAware();
+
+ if (!tracingEnabled && !applicationAware) {
+ tracer = OpenTelemetry.noop().getTracer(GLOBAL_TRACER_NAME);
+ return;
+ }
+
+ // Server reconfiguration reprioritizes Ozone's SDK over any adopted
global,
+ // and re-registers the global name and tracer.
+ if (preferOzone && tracingEnabled) {
+ initOzoneSdk(serviceName, cfg, true);
+ return;
+ }
+
+ // Global first: adopt an application-registered GlobalOpenTelemetry when
present.
+ if (GlobalOpenTelemetry.isSet() &&
isRealGlobal(GlobalOpenTelemetry.get())) {
+ tracer = GlobalOpenTelemetry.get().getTracer(GLOBAL_TRACER_NAME);
+ LOG.info("Tracing: adopted application GlobalOpenTelemetry");
+ return;
+ }
+
+ initOzoneSdk(serviceName, cfg, tracingEnabled);
+ }
+
+ private static void initOzoneSdk(String serviceName, TracingConfig cfg,
boolean registerGlobal) {
+ SdkTracerProvider tracerProvider = buildSdkTracerProvider(serviceName,
cfg);
+ try {
+ OpenTelemetrySdk sdk;
+ if (registerGlobal) {
+ sdk = OpenTelemetrySdk.builder()
+ .setTracerProvider(tracerProvider)
+
.setPropagators(ContextPropagators.create(W3CTraceContextPropagator.getInstance()))
+ .build();
+ // GlobalOpenTelemetry.set is one-shot
+ GlobalOpenTelemetry.resetForTest();
+ GlobalOpenTelemetry.set(sdk);
+ tracer = GlobalOpenTelemetry.get().getTracer(GLOBAL_TRACER_NAME);
+ } else {
+ sdk = OpenTelemetrySdk.builder()
+ .setTracerProvider(tracerProvider)
+ .build();
+ tracer = sdk.getTracer(GLOBAL_TRACER_NAME);
+ }
+ sdkTracerProvider = tracerProvider;
+ } catch (RuntimeException e) {
+ tracerProvider.shutdown();
+ batchSpanProcessor = null;
+ throw e;
+ }
+ }
+
+ /**
+ * Distinguish an application-registered GlobalOpenTelemetry from the OTel
built-in noop.
+ * OpenTelemetry.noop() returns a singleton, so identity comparison is
sufficient.
+ */
+ private static boolean isRealGlobal(OpenTelemetry global) {
+ return global != null && global != OpenTelemetry.noop();
+ }
+
+ /**
+ * Build the SdkTracerProvider using the configured OTLP endpoint and
sampler.
+ * Extracted so both enabled and application-aware modes share
exporter/sampler setup.
+ */
+ private static SdkTracerProvider buildSdkTracerProvider(
+ String serviceName, TracingConfig tracingConfig) {
//Fetch and log the right tracing parameters based on config, environment
variable and default value priority.
String otelEndPoint = tracingConfig.getTracingEndpoint();
Review Comment:
test if endpoint is configured empty or invalid value, what is the 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]
