ctubbsii commented on a change in pull request #2259:
URL: https://github.com/apache/accumulo/pull/2259#discussion_r745113391
##########
File path: core/pom.xml
##########
@@ -262,6 +270,7 @@
</includes>
<excludes />
<allows>
+ <allow>io[.]opentelemetry[.]api[.]OpenTelemetry</allow>
Review comment:
This would allow using OpenTelemetry objects in Accumulo's own public
API. This is needed because we have the OpenTelemetryFactory SPI. However, I'm
second guessing the need for that, as it is generally undesirable to expose
3rd-party types in our own public API, if we can help it.
As an alternative, consider that OpenTelemetryFactory is nothing more than
`Supplier<OpenTelemetry>`. As such, instead of having our own dedicated SPI
type, we can just document that the implementing class be of type
`Supplier<OpenTelemetry>`. I'm not sure this is the *best* idea, but it would
have the advantage of keeping new 3rd-party types out of our API/SPI.
Given the complexity of this, I'm also starting to wonder if we shouldn't
just restrict the OpenTelemetry instance to whatever is set in the
GlobalOpenTelemetry class, and just rely on users to use a JavaAgent or
something to set it if they really want to, rather than even be given the
option of configuring it via our code.
##########
File path: assemble/conf/accumulo-env.sh
##########
@@ -92,10 +92,23 @@ JAVA_OPTS=("${JAVA_OPTS[@]}"
"-Daccumulo.application=${cmd}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)"
"-Daccumulo.metrics.service.instance=${ACCUMULO_SERVICE_INSTANCE}"
"-Dlog4j2.contextSelector=org.apache.logging.log4j.core.async.AsyncLoggerContextSelector"
+ "-Dotel.service.name=${cmd}${ACCUMULO_SERVICE_INSTANCE}"
)
+## Optionally setup OpenTelemetry SDK AutoConfigure
+## See
https://github.com/open-telemetry/opentelemetry-java/tree/main/sdk-extensions/autoconfigure
+#JAVA_OPTS=("${JAVA_OPTS[@]}"
+# "-Dotel.traces.exporter=jaeger"
+#)
Review comment:
This can take up less space
```suggestion
#JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dotel.traces.exporter=jaeger")
```
##########
File path: assemble/conf/accumulo-env.sh
##########
@@ -92,10 +92,23 @@ JAVA_OPTS=("${JAVA_OPTS[@]}"
"-Daccumulo.application=${cmd}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)"
"-Daccumulo.metrics.service.instance=${ACCUMULO_SERVICE_INSTANCE}"
"-Dlog4j2.contextSelector=org.apache.logging.log4j.core.async.AsyncLoggerContextSelector"
+ "-Dotel.service.name=${cmd}${ACCUMULO_SERVICE_INSTANCE}"
)
+## Optionally setup OpenTelemetry SDK AutoConfigure
+## See
https://github.com/open-telemetry/opentelemetry-java/tree/main/sdk-extensions/autoconfigure
+#JAVA_OPTS=("${JAVA_OPTS[@]}"
+# "-Dotel.traces.exporter=jaeger"
+#)
+
+## Optionally setup OpenTelemetry Java Agent
+## See https://github.com/open-telemetry/opentelemetry-java-instrumentation
for more options
+#JAVA_OPTS=("${JAVA_OPTS[@]}"
+# "-javaagent:path/to/opentelemetry-javaagent-all.jar"
+#)
Review comment:
```suggestion
#JAVA_OPTS=("${JAVA_OPTS[@]}"
"-javaagent:path/to/opentelemetry-javaagent-all.jar")
```
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -763,6 +771,7 @@
"The number of threads used to delete RFiles and write-ahead logs",
"1.3.5"),
GC_TRASH_IGNORE("gc.trash.ignore", "false", PropertyType.BOOLEAN,
"Do not use the Trash, even if it is configured.", "1.5.0"),
+ @Deprecated(since = "2.1.0", forRemoval = true)
Review comment:
These properties inconsistently use `forRemoval`. I like using it if we
can get away with it, but in some cases, it can cause problems for certain IDEs
(https://bugs.eclipse.org/bugs/show_bug.cgi?id=565271). If we can consistently
use it without causing problems, I think that's better, but if we can't, it
might be better to prioritize consistency over maximal usage of `forRemoval`
where we can and avoiding it where we can't.
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -18,211 +18,230 @@
*/
package org.apache.accumulo.core.trace;
-import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
import java.util.Map;
-import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.Objects;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
-import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.spi.trace.OpenTelemetryFactory;
import org.apache.accumulo.core.trace.thrift.TInfo;
-import org.apache.hadoop.util.ShutdownHookManager;
-import org.apache.htrace.HTraceConfiguration;
-import org.apache.htrace.NullScope;
-import org.apache.htrace.Span;
-import org.apache.htrace.SpanReceiver;
-import org.apache.htrace.SpanReceiverBuilder;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceInfo;
-import org.apache.htrace.TraceScope;
-import org.apache.htrace.impl.CountSampler;
-import org.apache.htrace.impl.ProbabilitySampler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/**
- * Utility class for tracing within Accumulo. Not intended for client use!
- */
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+
public class TraceUtil {
- private static final Logger log = LoggerFactory.getLogger(TraceUtil.class);
- public static final String TRACE_HOST_PROPERTY = "trace.host";
- public static final String TRACE_SERVICE_PROPERTY = "trace.service";
- public static final String TRACER_ZK_HOST = "tracer.zookeeper.host";
- public static final String TRACER_ZK_TIMEOUT = "tracer.zookeeper.timeout";
- public static final String TRACER_ZK_PATH = "tracer.zookeeper.path";
- private static final HashSet<SpanReceiver> receivers = new HashSet<>();
+ public static final Logger LOG = LoggerFactory.getLogger(TraceUtil.class);
+
+ private static final String SPAN_FORMAT = "%s::%s";
+
+ private static Tracer instance = null;
+ private static String name = null;
+ private static boolean tracing = false;
+
+ private static void initializeInternals(OpenTelemetry ot, String
instrumentationName) {
+ // TODO: Is there a way to get our version to pass to getTracer() ?
+ name = instrumentationName;
+ instance = ot.getTracer(name);
+ tracing = (!ot.equals(OpenTelemetry.noop()));
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance:
{}", tracing,
+ ot.getClass(), instance.getClass());
+ }
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
- * Properties required in the client configuration include
- * {@link org.apache.accumulo.core.conf.ClientProperty#TRACE_SPAN_RECEIVERS}
and any properties
- * specific to the span receiver.
+ * Initialize TracerUtil using the OpenTelemetry parameter and
instrumentationName
+ *
+ * @param ot
+ * OpenTelemetry instance
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
*/
- public static void enableClientTraces(String hostname, String service,
Properties properties) {
- // @formatter:off
- enableTracing(hostname, service,
- ClientProperty.TRACE_SPAN_RECEIVERS.getValue(properties),
- ClientProperty.INSTANCE_ZOOKEEPERS.getValue(properties),
- ConfigurationTypeHelper
-
.getTimeInMillis(ClientProperty.INSTANCE_ZOOKEEPERS_TIMEOUT.getValue(properties)),
- ClientProperty.TRACE_ZOOKEEPER_PATH.getValue(properties),
- ClientProperty.toMap(
- ClientProperty.getPrefix(properties,
ClientProperty.TRACE_SPAN_RECEIVER_PREFIX)));
- // @formatter:on
+ public static void initializeTracer(OpenTelemetry ot, String
instrumentationName) {
+ if (instance != null) {
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
+ }
}
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
+ * Use the property values in the AccumuloConfiguration to call
+ * {@link #initializeTracer(boolean, String, String)}
+ *
+ * @param conf
+ * AccumuloConfiguration
+ * @throws Exception
+ * unable to find or load class
*/
- public static void enableServerTraces(String hostname, String service,
- AccumuloConfiguration conf) {
- // @formatter:off
- enableTracing(hostname, service,
- conf.get(Property.TRACE_SPAN_RECEIVERS),
- conf.get(Property.INSTANCE_ZK_HOST),
- conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
- conf.get(Property.TRACE_ZK_PATH),
-
conf.getAllPropertiesWithPrefix(Property.TRACE_SPAN_RECEIVER_PREFIX));
- // @formatter:on
+ public static void initializeTracer(AccumuloConfiguration conf) throws
Exception {
+ initializeTracer(conf.getBoolean(Property.GENERAL_OPENTELEMETRY_ENABLED),
+ conf.get(Property.GENERAL_OPENTELEMETRY_FACTORY),
+ conf.get(Property.GENERAL_OPENTELEMETRY_NAME));
}
- private static void enableTracing(String hostname, String service, String
spanReceivers,
- String zookeepers, long timeout, String zkPath, Map<String,String>
spanReceiverProps) {
-
- Map<String,
- String> htraceConfigProps =
spanReceiverProps.entrySet().stream().collect(Collectors.toMap(
- k ->
String.valueOf(k).substring(Property.TRACE_SPAN_RECEIVER_PREFIX.getKey().length()),
- String::valueOf, (a, b) -> {
- throw new AssertionError("duplicate can't happen");
- }, HashMap::new));
- htraceConfigProps.put(TRACER_ZK_HOST, zookeepers);
- htraceConfigProps.put(TRACER_ZK_TIMEOUT, Long.toString(timeout));
- htraceConfigProps.put(TRACER_ZK_PATH, zkPath);
- if (hostname != null) {
- htraceConfigProps.put(TRACE_HOST_PROPERTY, hostname);
- }
- if (service != null) {
- htraceConfigProps.put(TRACE_SERVICE_PROPERTY, service);
- }
- ShutdownHookManager.get().addShutdownHook(TraceUtil::disable, 0);
- synchronized (receivers) {
- if (!receivers.isEmpty()) {
- log.info("Already loaded span receivers, enable tracing does not need
to be called again");
- return;
- }
- if (spanReceivers == null) {
- return;
- }
- Stream.of(spanReceivers.split(",")).map(String::trim).filter(s ->
!s.isEmpty())
- .forEach(className -> {
- HTraceConfiguration htraceConf =
HTraceConfiguration.fromMap(htraceConfigProps);
- SpanReceiverBuilder builder = new SpanReceiverBuilder(htraceConf);
- SpanReceiver rcvr =
builder.spanReceiverClass(className.trim()).build();
- if (rcvr == null) {
- log.warn("Failed to load SpanReceiver {}", className);
- } else {
- receivers.add(rcvr);
- log.debug("SpanReceiver {} was loaded successfully.", className);
- }
- });
- for (SpanReceiver rcvr : receivers) {
- Trace.addReceiver(rcvr);
+ /**
+ * If not enabled, the OpenTelemetry implementation will be set to the NoOp
implementation. If
+ * enabled and a factoryClass is supplied, then we will get the
OpenTelemetry instance from the
+ * factory class.
+ *
+ * @param enabled
+ * whether or not tracing is enabled
+ * @param factoryClass
+ * name of class to load
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
+ * @throws Exception
+ * unable to find or load class
+ */
+ public static void initializeTracer(boolean enabled, String factoryClass,
+ String instrumentationName) throws Exception {
+ if (instance == null) {
+ OpenTelemetry ot = null;
+ if (!enabled) {
+ ot = OpenTelemetry.noop();
+ } else if (factoryClass != null && !factoryClass.isEmpty()) {
+ Class<? extends OpenTelemetryFactory> clazz =
+ (Class<? extends OpenTelemetryFactory>)
ClassLoaderUtil.loadClass(factoryClass,
+ OpenTelemetryFactory.class);
Review comment:
The cast is redundant since loadClass is a generic method that returns
the appropriate type based on the provided class parameter. In this case, it's
probably fine to just use `var` as well.
```suggestion
var clazz = ClassLoaderUtil.loadClass(factoryClass,
OpenTelemetryFactory.class);
```
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -18,211 +18,230 @@
*/
package org.apache.accumulo.core.trace;
-import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
import java.util.Map;
-import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.Objects;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
-import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.spi.trace.OpenTelemetryFactory;
import org.apache.accumulo.core.trace.thrift.TInfo;
-import org.apache.hadoop.util.ShutdownHookManager;
-import org.apache.htrace.HTraceConfiguration;
-import org.apache.htrace.NullScope;
-import org.apache.htrace.Span;
-import org.apache.htrace.SpanReceiver;
-import org.apache.htrace.SpanReceiverBuilder;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceInfo;
-import org.apache.htrace.TraceScope;
-import org.apache.htrace.impl.CountSampler;
-import org.apache.htrace.impl.ProbabilitySampler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/**
- * Utility class for tracing within Accumulo. Not intended for client use!
- */
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+
public class TraceUtil {
- private static final Logger log = LoggerFactory.getLogger(TraceUtil.class);
- public static final String TRACE_HOST_PROPERTY = "trace.host";
- public static final String TRACE_SERVICE_PROPERTY = "trace.service";
- public static final String TRACER_ZK_HOST = "tracer.zookeeper.host";
- public static final String TRACER_ZK_TIMEOUT = "tracer.zookeeper.timeout";
- public static final String TRACER_ZK_PATH = "tracer.zookeeper.path";
- private static final HashSet<SpanReceiver> receivers = new HashSet<>();
+ public static final Logger LOG = LoggerFactory.getLogger(TraceUtil.class);
+
+ private static final String SPAN_FORMAT = "%s::%s";
+
+ private static Tracer instance = null;
+ private static String name = null;
+ private static boolean tracing = false;
+
+ private static void initializeInternals(OpenTelemetry ot, String
instrumentationName) {
+ // TODO: Is there a way to get our version to pass to getTracer() ?
Review comment:
I'm not sure I understand this comment. What version are you referring
to?
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -18,211 +18,230 @@
*/
package org.apache.accumulo.core.trace;
-import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
import java.util.Map;
-import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.Objects;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
-import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.spi.trace.OpenTelemetryFactory;
import org.apache.accumulo.core.trace.thrift.TInfo;
-import org.apache.hadoop.util.ShutdownHookManager;
-import org.apache.htrace.HTraceConfiguration;
-import org.apache.htrace.NullScope;
-import org.apache.htrace.Span;
-import org.apache.htrace.SpanReceiver;
-import org.apache.htrace.SpanReceiverBuilder;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceInfo;
-import org.apache.htrace.TraceScope;
-import org.apache.htrace.impl.CountSampler;
-import org.apache.htrace.impl.ProbabilitySampler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/**
- * Utility class for tracing within Accumulo. Not intended for client use!
- */
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+
public class TraceUtil {
- private static final Logger log = LoggerFactory.getLogger(TraceUtil.class);
- public static final String TRACE_HOST_PROPERTY = "trace.host";
- public static final String TRACE_SERVICE_PROPERTY = "trace.service";
- public static final String TRACER_ZK_HOST = "tracer.zookeeper.host";
- public static final String TRACER_ZK_TIMEOUT = "tracer.zookeeper.timeout";
- public static final String TRACER_ZK_PATH = "tracer.zookeeper.path";
- private static final HashSet<SpanReceiver> receivers = new HashSet<>();
+ public static final Logger LOG = LoggerFactory.getLogger(TraceUtil.class);
+
+ private static final String SPAN_FORMAT = "%s::%s";
+
+ private static Tracer instance = null;
+ private static String name = null;
+ private static boolean tracing = false;
+
+ private static void initializeInternals(OpenTelemetry ot, String
instrumentationName) {
+ // TODO: Is there a way to get our version to pass to getTracer() ?
+ name = instrumentationName;
+ instance = ot.getTracer(name);
+ tracing = (!ot.equals(OpenTelemetry.noop()));
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance:
{}", tracing,
+ ot.getClass(), instance.getClass());
+ }
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
- * Properties required in the client configuration include
- * {@link org.apache.accumulo.core.conf.ClientProperty#TRACE_SPAN_RECEIVERS}
and any properties
- * specific to the span receiver.
+ * Initialize TracerUtil using the OpenTelemetry parameter and
instrumentationName
+ *
+ * @param ot
+ * OpenTelemetry instance
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
*/
- public static void enableClientTraces(String hostname, String service,
Properties properties) {
- // @formatter:off
- enableTracing(hostname, service,
- ClientProperty.TRACE_SPAN_RECEIVERS.getValue(properties),
- ClientProperty.INSTANCE_ZOOKEEPERS.getValue(properties),
- ConfigurationTypeHelper
-
.getTimeInMillis(ClientProperty.INSTANCE_ZOOKEEPERS_TIMEOUT.getValue(properties)),
- ClientProperty.TRACE_ZOOKEEPER_PATH.getValue(properties),
- ClientProperty.toMap(
- ClientProperty.getPrefix(properties,
ClientProperty.TRACE_SPAN_RECEIVER_PREFIX)));
- // @formatter:on
+ public static void initializeTracer(OpenTelemetry ot, String
instrumentationName) {
+ if (instance != null) {
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
+ }
}
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
+ * Use the property values in the AccumuloConfiguration to call
+ * {@link #initializeTracer(boolean, String, String)}
+ *
+ * @param conf
+ * AccumuloConfiguration
+ * @throws Exception
+ * unable to find or load class
*/
- public static void enableServerTraces(String hostname, String service,
- AccumuloConfiguration conf) {
- // @formatter:off
- enableTracing(hostname, service,
- conf.get(Property.TRACE_SPAN_RECEIVERS),
- conf.get(Property.INSTANCE_ZK_HOST),
- conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
- conf.get(Property.TRACE_ZK_PATH),
-
conf.getAllPropertiesWithPrefix(Property.TRACE_SPAN_RECEIVER_PREFIX));
- // @formatter:on
+ public static void initializeTracer(AccumuloConfiguration conf) throws
Exception {
+ initializeTracer(conf.getBoolean(Property.GENERAL_OPENTELEMETRY_ENABLED),
+ conf.get(Property.GENERAL_OPENTELEMETRY_FACTORY),
+ conf.get(Property.GENERAL_OPENTELEMETRY_NAME));
}
- private static void enableTracing(String hostname, String service, String
spanReceivers,
- String zookeepers, long timeout, String zkPath, Map<String,String>
spanReceiverProps) {
-
- Map<String,
- String> htraceConfigProps =
spanReceiverProps.entrySet().stream().collect(Collectors.toMap(
- k ->
String.valueOf(k).substring(Property.TRACE_SPAN_RECEIVER_PREFIX.getKey().length()),
- String::valueOf, (a, b) -> {
- throw new AssertionError("duplicate can't happen");
- }, HashMap::new));
- htraceConfigProps.put(TRACER_ZK_HOST, zookeepers);
- htraceConfigProps.put(TRACER_ZK_TIMEOUT, Long.toString(timeout));
- htraceConfigProps.put(TRACER_ZK_PATH, zkPath);
- if (hostname != null) {
- htraceConfigProps.put(TRACE_HOST_PROPERTY, hostname);
- }
- if (service != null) {
- htraceConfigProps.put(TRACE_SERVICE_PROPERTY, service);
- }
- ShutdownHookManager.get().addShutdownHook(TraceUtil::disable, 0);
- synchronized (receivers) {
- if (!receivers.isEmpty()) {
- log.info("Already loaded span receivers, enable tracing does not need
to be called again");
- return;
- }
- if (spanReceivers == null) {
- return;
- }
- Stream.of(spanReceivers.split(",")).map(String::trim).filter(s ->
!s.isEmpty())
- .forEach(className -> {
- HTraceConfiguration htraceConf =
HTraceConfiguration.fromMap(htraceConfigProps);
- SpanReceiverBuilder builder = new SpanReceiverBuilder(htraceConf);
- SpanReceiver rcvr =
builder.spanReceiverClass(className.trim()).build();
- if (rcvr == null) {
- log.warn("Failed to load SpanReceiver {}", className);
- } else {
- receivers.add(rcvr);
- log.debug("SpanReceiver {} was loaded successfully.", className);
- }
- });
- for (SpanReceiver rcvr : receivers) {
- Trace.addReceiver(rcvr);
+ /**
+ * If not enabled, the OpenTelemetry implementation will be set to the NoOp
implementation. If
+ * enabled and a factoryClass is supplied, then we will get the
OpenTelemetry instance from the
+ * factory class.
+ *
+ * @param enabled
+ * whether or not tracing is enabled
+ * @param factoryClass
+ * name of class to load
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
+ * @throws Exception
+ * unable to find or load class
+ */
+ public static void initializeTracer(boolean enabled, String factoryClass,
+ String instrumentationName) throws Exception {
+ if (instance == null) {
+ OpenTelemetry ot = null;
+ if (!enabled) {
+ ot = OpenTelemetry.noop();
+ } else if (factoryClass != null && !factoryClass.isEmpty()) {
+ Class<? extends OpenTelemetryFactory> clazz =
+ (Class<? extends OpenTelemetryFactory>)
ClassLoaderUtil.loadClass(factoryClass,
+ OpenTelemetryFactory.class);
+ OpenTelemetryFactory factory =
clazz.getDeclaredConstructor().newInstance();
+ ot = factory.getOpenTelemetry();
+ LOG.info("OpenTelemetry configured and set from {}", clazz);
+ } else {
+ ot = GlobalOpenTelemetry.get();
}
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
}
}
/**
- * Disable tracing by closing SpanReceivers for the current process.
+ * @return the Tracer set on the GlobalOpenTelemetry object
*/
- public static void disable() {
- synchronized (receivers) {
- receivers.forEach(rcvr -> {
- try {
- rcvr.close();
- } catch (IOException e) {
- log.warn("Unable to close SpanReceiver correctly: {}",
e.getMessage(), e);
- }
- });
- receivers.clear();
+ private static Tracer getTracer() {
+ if (Objects.isNull(instance)) {
Review comment:
It's more obvious what is happening when `== null` is used directly,
instead of adding to the call stack to do the check indirectly in another
method. `Objects.isNull` is primarily useful as a Predicate, as in
`someObj.removeIf(Objects::isNull)` rather than as a substitute for `== null`.
```suggestion
if (instance == null) {
```
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -18,211 +18,230 @@
*/
package org.apache.accumulo.core.trace;
-import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
import java.util.Map;
-import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.Objects;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
-import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.spi.trace.OpenTelemetryFactory;
import org.apache.accumulo.core.trace.thrift.TInfo;
-import org.apache.hadoop.util.ShutdownHookManager;
-import org.apache.htrace.HTraceConfiguration;
-import org.apache.htrace.NullScope;
-import org.apache.htrace.Span;
-import org.apache.htrace.SpanReceiver;
-import org.apache.htrace.SpanReceiverBuilder;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceInfo;
-import org.apache.htrace.TraceScope;
-import org.apache.htrace.impl.CountSampler;
-import org.apache.htrace.impl.ProbabilitySampler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/**
- * Utility class for tracing within Accumulo. Not intended for client use!
- */
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+
public class TraceUtil {
- private static final Logger log = LoggerFactory.getLogger(TraceUtil.class);
- public static final String TRACE_HOST_PROPERTY = "trace.host";
- public static final String TRACE_SERVICE_PROPERTY = "trace.service";
- public static final String TRACER_ZK_HOST = "tracer.zookeeper.host";
- public static final String TRACER_ZK_TIMEOUT = "tracer.zookeeper.timeout";
- public static final String TRACER_ZK_PATH = "tracer.zookeeper.path";
- private static final HashSet<SpanReceiver> receivers = new HashSet<>();
+ public static final Logger LOG = LoggerFactory.getLogger(TraceUtil.class);
+
+ private static final String SPAN_FORMAT = "%s::%s";
+
+ private static Tracer instance = null;
+ private static String name = null;
+ private static boolean tracing = false;
+
+ private static void initializeInternals(OpenTelemetry ot, String
instrumentationName) {
+ // TODO: Is there a way to get our version to pass to getTracer() ?
+ name = instrumentationName;
+ instance = ot.getTracer(name);
+ tracing = (!ot.equals(OpenTelemetry.noop()));
Review comment:
```suggestion
tracing = !ot.equals(OpenTelemetry.noop());
```
##########
File path:
core/src/test/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParserTest.java
##########
@@ -48,13 +48,12 @@ public void testParse() throws Exception {
Map<String,String> contents =
ClusterConfigParser.parseConfiguration(new
File(configFile.toURI()).getAbsolutePath());
- assertEquals(5, contents.size());
+ assertEquals(4, contents.size());
assertTrue(contents.containsKey("manager"));
assertEquals("localhost1 localhost2", contents.get("manager"));
assertTrue(contents.containsKey("monitor"));
assertEquals("localhost1 localhost2", contents.get("monitor"));
- assertTrue(contents.containsKey("tracer"));
- assertEquals("localhost", contents.get("tracer"));
+ assertFalse(contents.containsKey("tracer"));
Review comment:
Not really needed. We already checked that the contents are size 4, and
we have a separate assert for each of those 4.
```suggestion
```
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -18,211 +18,230 @@
*/
package org.apache.accumulo.core.trace;
-import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
import java.util.Map;
-import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.Objects;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
-import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.spi.trace.OpenTelemetryFactory;
import org.apache.accumulo.core.trace.thrift.TInfo;
-import org.apache.hadoop.util.ShutdownHookManager;
-import org.apache.htrace.HTraceConfiguration;
-import org.apache.htrace.NullScope;
-import org.apache.htrace.Span;
-import org.apache.htrace.SpanReceiver;
-import org.apache.htrace.SpanReceiverBuilder;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceInfo;
-import org.apache.htrace.TraceScope;
-import org.apache.htrace.impl.CountSampler;
-import org.apache.htrace.impl.ProbabilitySampler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/**
- * Utility class for tracing within Accumulo. Not intended for client use!
- */
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+
public class TraceUtil {
- private static final Logger log = LoggerFactory.getLogger(TraceUtil.class);
- public static final String TRACE_HOST_PROPERTY = "trace.host";
- public static final String TRACE_SERVICE_PROPERTY = "trace.service";
- public static final String TRACER_ZK_HOST = "tracer.zookeeper.host";
- public static final String TRACER_ZK_TIMEOUT = "tracer.zookeeper.timeout";
- public static final String TRACER_ZK_PATH = "tracer.zookeeper.path";
- private static final HashSet<SpanReceiver> receivers = new HashSet<>();
+ public static final Logger LOG = LoggerFactory.getLogger(TraceUtil.class);
+
+ private static final String SPAN_FORMAT = "%s::%s";
+
+ private static Tracer instance = null;
+ private static String name = null;
+ private static boolean tracing = false;
+
+ private static void initializeInternals(OpenTelemetry ot, String
instrumentationName) {
+ // TODO: Is there a way to get our version to pass to getTracer() ?
+ name = instrumentationName;
+ instance = ot.getTracer(name);
+ tracing = (!ot.equals(OpenTelemetry.noop()));
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance:
{}", tracing,
+ ot.getClass(), instance.getClass());
+ }
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
- * Properties required in the client configuration include
- * {@link org.apache.accumulo.core.conf.ClientProperty#TRACE_SPAN_RECEIVERS}
and any properties
- * specific to the span receiver.
+ * Initialize TracerUtil using the OpenTelemetry parameter and
instrumentationName
+ *
+ * @param ot
+ * OpenTelemetry instance
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
*/
- public static void enableClientTraces(String hostname, String service,
Properties properties) {
- // @formatter:off
- enableTracing(hostname, service,
- ClientProperty.TRACE_SPAN_RECEIVERS.getValue(properties),
- ClientProperty.INSTANCE_ZOOKEEPERS.getValue(properties),
- ConfigurationTypeHelper
-
.getTimeInMillis(ClientProperty.INSTANCE_ZOOKEEPERS_TIMEOUT.getValue(properties)),
- ClientProperty.TRACE_ZOOKEEPER_PATH.getValue(properties),
- ClientProperty.toMap(
- ClientProperty.getPrefix(properties,
ClientProperty.TRACE_SPAN_RECEIVER_PREFIX)));
- // @formatter:on
+ public static void initializeTracer(OpenTelemetry ot, String
instrumentationName) {
+ if (instance != null) {
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
+ }
}
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
+ * Use the property values in the AccumuloConfiguration to call
+ * {@link #initializeTracer(boolean, String, String)}
+ *
+ * @param conf
+ * AccumuloConfiguration
+ * @throws Exception
+ * unable to find or load class
*/
- public static void enableServerTraces(String hostname, String service,
- AccumuloConfiguration conf) {
- // @formatter:off
- enableTracing(hostname, service,
- conf.get(Property.TRACE_SPAN_RECEIVERS),
- conf.get(Property.INSTANCE_ZK_HOST),
- conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
- conf.get(Property.TRACE_ZK_PATH),
-
conf.getAllPropertiesWithPrefix(Property.TRACE_SPAN_RECEIVER_PREFIX));
- // @formatter:on
+ public static void initializeTracer(AccumuloConfiguration conf) throws
Exception {
+ initializeTracer(conf.getBoolean(Property.GENERAL_OPENTELEMETRY_ENABLED),
+ conf.get(Property.GENERAL_OPENTELEMETRY_FACTORY),
+ conf.get(Property.GENERAL_OPENTELEMETRY_NAME));
}
- private static void enableTracing(String hostname, String service, String
spanReceivers,
- String zookeepers, long timeout, String zkPath, Map<String,String>
spanReceiverProps) {
-
- Map<String,
- String> htraceConfigProps =
spanReceiverProps.entrySet().stream().collect(Collectors.toMap(
- k ->
String.valueOf(k).substring(Property.TRACE_SPAN_RECEIVER_PREFIX.getKey().length()),
- String::valueOf, (a, b) -> {
- throw new AssertionError("duplicate can't happen");
- }, HashMap::new));
- htraceConfigProps.put(TRACER_ZK_HOST, zookeepers);
- htraceConfigProps.put(TRACER_ZK_TIMEOUT, Long.toString(timeout));
- htraceConfigProps.put(TRACER_ZK_PATH, zkPath);
- if (hostname != null) {
- htraceConfigProps.put(TRACE_HOST_PROPERTY, hostname);
- }
- if (service != null) {
- htraceConfigProps.put(TRACE_SERVICE_PROPERTY, service);
- }
- ShutdownHookManager.get().addShutdownHook(TraceUtil::disable, 0);
- synchronized (receivers) {
- if (!receivers.isEmpty()) {
- log.info("Already loaded span receivers, enable tracing does not need
to be called again");
- return;
- }
- if (spanReceivers == null) {
- return;
- }
- Stream.of(spanReceivers.split(",")).map(String::trim).filter(s ->
!s.isEmpty())
- .forEach(className -> {
- HTraceConfiguration htraceConf =
HTraceConfiguration.fromMap(htraceConfigProps);
- SpanReceiverBuilder builder = new SpanReceiverBuilder(htraceConf);
- SpanReceiver rcvr =
builder.spanReceiverClass(className.trim()).build();
- if (rcvr == null) {
- log.warn("Failed to load SpanReceiver {}", className);
- } else {
- receivers.add(rcvr);
- log.debug("SpanReceiver {} was loaded successfully.", className);
- }
- });
- for (SpanReceiver rcvr : receivers) {
- Trace.addReceiver(rcvr);
+ /**
+ * If not enabled, the OpenTelemetry implementation will be set to the NoOp
implementation. If
+ * enabled and a factoryClass is supplied, then we will get the
OpenTelemetry instance from the
+ * factory class.
+ *
+ * @param enabled
+ * whether or not tracing is enabled
+ * @param factoryClass
+ * name of class to load
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
+ * @throws Exception
+ * unable to find or load class
+ */
+ public static void initializeTracer(boolean enabled, String factoryClass,
+ String instrumentationName) throws Exception {
+ if (instance == null) {
+ OpenTelemetry ot = null;
+ if (!enabled) {
+ ot = OpenTelemetry.noop();
+ } else if (factoryClass != null && !factoryClass.isEmpty()) {
+ Class<? extends OpenTelemetryFactory> clazz =
+ (Class<? extends OpenTelemetryFactory>)
ClassLoaderUtil.loadClass(factoryClass,
+ OpenTelemetryFactory.class);
+ OpenTelemetryFactory factory =
clazz.getDeclaredConstructor().newInstance();
+ ot = factory.getOpenTelemetry();
+ LOG.info("OpenTelemetry configured and set from {}", clazz);
+ } else {
+ ot = GlobalOpenTelemetry.get();
}
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
}
}
/**
- * Disable tracing by closing SpanReceivers for the current process.
+ * @return the Tracer set on the GlobalOpenTelemetry object
*/
- public static void disable() {
- synchronized (receivers) {
- receivers.forEach(rcvr -> {
- try {
- rcvr.close();
- } catch (IOException e) {
- log.warn("Unable to close SpanReceiver correctly: {}",
e.getMessage(), e);
- }
- });
- receivers.clear();
+ private static Tracer getTracer() {
+ if (Objects.isNull(instance)) {
+ LOG.warn("initializeTracer not called, using
GlobalOpenTelemetry.getTracer()");
+ instance = GlobalOpenTelemetry.getTracer(name);
+ tracing = (!instance.equals(OpenTelemetry.noop().getTracer(name)));
+ LOG.info("Trace enabled: {}, Tracer is: {}", tracing,
instance.getClass());
}
+ return instance;
}
/**
- * Continue a trace by starting a new span with a given parent and
description.
+ * @return true if an OpenTelemetry Tracer implementation has been set,
false if the NoOp Tracer
+ * is being used.
*/
- public static TraceScope trace(TInfo info, String description) {
- return info.traceId == 0 ? NullScope.INSTANCE
- : Trace.startSpan(description, new TraceInfo(info.traceId,
info.parentId));
+ public static boolean isTracing() {
+ return tracing;
}
- private static final TInfo DONT_TRACE = new TInfo(0, 0);
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind) {
+ return createSpan(caller, spanName, kind, null, false, null);
+ }
- /**
- * Obtain {@link org.apache.accumulo.core.trace.thrift.TInfo} for the
current span.
- */
- public static TInfo traceInfo() {
- Span span = Trace.currentSpan();
- if (span != null) {
- return new TInfo(span.getTraceId(), span.getSpanId());
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind, Context parent) {
+ return createSpan(caller, spanName, kind, null, false, parent);
+ }
+
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind,
+ Map<String,String> attributes) {
+ return createSpan(caller, spanName, kind, attributes, false, null);
+ }
+
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind,
+ Map<String,String> attributes, boolean setNoParent, Context parent) {
+ final String name = String.format(SPAN_FORMAT, caller.getSimpleName(),
spanName);
+ final SpanBuilder builder = getTracer().spanBuilder(name);
+ builder.setSpanKind(kind);
+ if (attributes != null) {
+ attributes.forEach((k, v) -> builder.setAttribute(k, v));
}
- return DONT_TRACE;
+ if (setNoParent) {
+ builder.setNoParent();
+ } else if (parent != null) {
+ builder.setParent(parent);
+ }
+ return builder.startSpan();
}
- public static CountSampler countSampler(long frequency) {
- return new CountSampler(HTraceConfiguration.fromMap(Collections
- .singletonMap(CountSampler.SAMPLER_FREQUENCY_CONF_KEY,
Long.toString(frequency))));
+ /**
+ * Record that an Exception occurred in the code covered by a Span
+ *
+ * @param span
+ * the span
+ * @param e
+ * the exception
+ * @param rethrown
+ * whether the exception is subsequently re-thrown
+ */
+ public static void setException(Span span, Throwable e, boolean rethrown) {
+ if (tracing) {
+ span.setStatus(StatusCode.ERROR);
+ span.recordException(e,
+ Attributes.builder().put(SemanticAttributes.EXCEPTION_TYPE,
e.getClass().getName())
+ .put(SemanticAttributes.EXCEPTION_MESSAGE, e.getMessage())
Review comment:
I'm surprised the exception type and message aren't automatically
recorded by the fact that we've provided the actual exception, `e` as the first
param.
##########
File path:
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java
##########
@@ -133,12 +133,14 @@ MiniAccumuloConfigImpl initialize() {
zooKeeperDir = new File(dir, "zookeeper");
logDir = new File(dir, "logs");
+ @SuppressWarnings("deprecation")
+ final String traceTokenPropertyPrefixKey =
Property.TRACE_TOKEN_PROPERTY_PREFIX.getKey();
// Never want to override these if an existing instance, which may be
using the defaults
if (existingInstance == null || !existingInstance) {
existingInstance = false;
mergeProp(Property.INSTANCE_VOLUMES.getKey(), "file://" +
accumuloDir.getAbsolutePath());
mergeProp(Property.INSTANCE_SECRET.getKey(), DEFAULT_INSTANCE_SECRET);
- mergeProp(Property.TRACE_TOKEN_PROPERTY_PREFIX.getKey() + "password",
getRootPassword());
+ mergeProp(traceTokenPropertyPrefixKey + "password", getRootPassword());
Review comment:
We don't need to write this to the file at all anymore, since it is
unused now.
```suggestion
```
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java
##########
@@ -49,7 +51,11 @@ protected AbstractServer(String appName, ServerOpts opts,
String[] args) {
log.info("Instance " + context.getInstanceID());
context.init(appName);
ClassLoaderUtil.initContextFactory(context.getConfiguration());
- TraceUtil.enableServerTraces(hostname, appName,
context.getConfiguration());
+ try {
+ TraceUtil.initializeTracer(context.getConfiguration());
+ } catch (Exception e) {
+ log.error("Error initializing tracing", e);
+ }
Review comment:
TraceUtil could handle initialization errors itself, and not throw
anything.
##########
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/TraceCommand.java
##########
@@ -18,80 +18,34 @@
*/
package org.apache.accumulo.shell.commands;
-import static
org.apache.accumulo.fate.util.UtilWaitThread.sleepUninterruptibly;
-
import java.io.IOException;
-import java.util.Map;
-import java.util.concurrent.TimeUnit;
-import org.apache.accumulo.core.client.Scanner;
-import org.apache.accumulo.core.conf.Property;
-import org.apache.accumulo.core.data.Range;
-import org.apache.accumulo.core.security.Authorizations;
+import org.apache.accumulo.core.trace.TraceUtil;
import org.apache.accumulo.core.util.BadArgumentException;
import org.apache.accumulo.shell.Shell;
import org.apache.accumulo.shell.Shell.Command;
-import org.apache.accumulo.tracer.TraceDump;
import org.apache.commons.cli.CommandLine;
-import org.apache.hadoop.io.Text;
-import org.apache.htrace.Sampler;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceScope;
+
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanKind;
public class TraceCommand extends Command {
- private TraceScope traceScope = null;
+ private Span span = null;
@Override
public int execute(final String fullCommand, final CommandLine cl, final
Shell shellState)
throws IOException {
if (cl.getArgs().length == 1) {
if (cl.getArgs()[0].equalsIgnoreCase("on")) {
- if (traceScope == null) {
- traceScope =
- Trace.startSpan("shell:" +
shellState.getAccumuloClient().whoami(), Sampler.ALWAYS);
+ if (span == null) {
+ span = TraceUtil.createSpan(Shell.class,
shellState.getAccumuloClient().whoami(),
+ SpanKind.CLIENT);
}
} else if (cl.getArgs()[0].equalsIgnoreCase("off")) {
- if (traceScope != null) {
- final long trace = traceScope.getSpan().getTraceId();
- traceScope.close();
- traceScope = null;
- StringBuilder sb = new StringBuilder();
- int traceCount = 0;
- for (int i = 0; i < 30; i++) {
- sb = new StringBuilder();
- try {
- final Map<String,String> properties =
-
shellState.getAccumuloClient().instanceOperations().getSystemConfiguration();
- final String table =
properties.get(Property.TRACE_TABLE.getKey());
- final String user = shellState.getAccumuloClient().whoami();
- final Authorizations auths =
-
shellState.getAccumuloClient().securityOperations().getUserAuthorizations(user);
- final Scanner scanner =
shellState.getAccumuloClient().createScanner(table, auths);
- scanner.setRange(new Range(new Text(Long.toHexString(trace))));
- final StringBuilder finalSB = sb;
- traceCount = TraceDump.printTrace(scanner, line -> {
- try {
- finalSB.append(line + "\n");
- } catch (Exception ex) {
- throw new RuntimeException(ex);
- }
- });
- if (traceCount > 0) {
- shellState.getWriter().print(sb.toString());
- break;
- }
- } catch (Exception ex) {
- shellState.printException(ex);
- }
- shellState.getWriter().println("Waiting for trace information");
- shellState.getWriter().flush();
- sleepUninterruptibly(500, TimeUnit.MILLISECONDS);
- }
- if (traceCount < 0) {
- // display the trace even though there are unrooted spans
- shellState.getWriter().print(sb.toString());
- }
+ if (span != null) {
+ span.end();
+ span = null;
Review comment:
I don't think the changes to this class preserve what was originally
intended. I think what was originally intended was something like swapping out
the global tracer with the NOOP tracer when set to `off`, and using whatever
configured tracer was initialized if the tracer was set to `on`. If it were
possible to set the sampler here, we could just grab a Tracer configured with a
custom sampler that just read the value of a static AtomicBoolean in TraceUtil.
But, to set the sampler, we'd need to use the SDK, which limits the
pluggability of the OpenTelemetry instance.
##########
File path:
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java
##########
@@ -133,12 +133,14 @@ MiniAccumuloConfigImpl initialize() {
zooKeeperDir = new File(dir, "zookeeper");
logDir = new File(dir, "logs");
+ @SuppressWarnings("deprecation")
+ final String traceTokenPropertyPrefixKey =
Property.TRACE_TOKEN_PROPERTY_PREFIX.getKey();
Review comment:
```suggestion
```
##########
File path:
server/monitor/src/main/java/org/apache/accumulo/monitor/rest/trace/RecentTracesInformation.java
##########
@@ -51,25 +49,4 @@ public RecentTracesInformation(String type) {
this.type = type;
}
- /**
- * Adds a span for the trace
- *
- * @param span
- * Remote span to obtain information
- */
- public void addSpan(RemoteSpan span) {
- total++;
- long ms = span.stop - span.start;
- totalMS += ms;
Review comment:
totalMS is now unused and should be deleted also.
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -239,6 +239,12 @@
"1.6.5"),
GENERAL_MAX_MESSAGE_SIZE("general.server.message.size.max", "1G",
PropertyType.BYTES,
"The maximum size of a message that can be sent to a server.", "1.5.0"),
+ GENERAL_OPENTELEMETRY_ENABLED("general.opentelemetry.enabled", "false",
PropertyType.BOOLEAN,
+ "Enables tracing functionality using OpenTelemetry.", "2.1.0"),
+ GENERAL_OPENTELEMETRY_FACTORY("general.opentelemetry.factory", "",
PropertyType.CLASSNAME,
+ "Name of class that implements OpenTelemetryFactory", "2.1.0"),
+ GENERAL_OPENTELEMETRY_NAME("general.opentelemetry.instrumentation.name", "",
PropertyType.STRING,
+ "Name of the instrumentation library to use.", "2.1.0"),
Review comment:
I see that there is updated documentation at
https://opentelemetry.io/docs/java/manual_instrumentation/#tracing that further
explains, with links to a glossary the difference between the "instrumentation
library" and the "instrumented library", clarifying that they are the same in
the case of manual instrumentation, such as what we're doing. So, we can just
pass in "org.apache.accumulo" as the hard-coded name of the instrumentation and
our current version, when we call `getTracer(name, version)` from the
OpenTelemetry instance that is supplied to us.
There's also some interesting `@WithSpan` annotations that we could use from
the [OpenTelemetry extension
annotations](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/manual-instrumentation.md)
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -239,6 +239,12 @@
"1.6.5"),
GENERAL_MAX_MESSAGE_SIZE("general.server.message.size.max", "1G",
PropertyType.BYTES,
"The maximum size of a message that can be sent to a server.", "1.5.0"),
+ GENERAL_OPENTELEMETRY_ENABLED("general.opentelemetry.enabled", "false",
PropertyType.BOOLEAN,
+ "Enables tracing functionality using OpenTelemetry.", "2.1.0"),
+ GENERAL_OPENTELEMETRY_FACTORY("general.opentelemetry.factory", "",
PropertyType.CLASSNAME,
+ "Name of class that implements OpenTelemetryFactory", "2.1.0"),
+ GENERAL_OPENTELEMETRY_NAME("general.opentelemetry.instrumentation.name", "",
PropertyType.STRING,
+ "Name of the instrumentation library to use.", "2.1.0"),
Review comment:
Because OpenTelemetry is still somewhat new, and it's definitely new to
us, I think it might be a good idea to mark these as `@Experimental` for at
least a minor release, until we're highly confident that this set of properties
will be stable. Same goes for the corresponding client properties.
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -18,211 +18,230 @@
*/
package org.apache.accumulo.core.trace;
-import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
import java.util.Map;
-import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.Objects;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
-import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.spi.trace.OpenTelemetryFactory;
import org.apache.accumulo.core.trace.thrift.TInfo;
-import org.apache.hadoop.util.ShutdownHookManager;
-import org.apache.htrace.HTraceConfiguration;
-import org.apache.htrace.NullScope;
-import org.apache.htrace.Span;
-import org.apache.htrace.SpanReceiver;
-import org.apache.htrace.SpanReceiverBuilder;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceInfo;
-import org.apache.htrace.TraceScope;
-import org.apache.htrace.impl.CountSampler;
-import org.apache.htrace.impl.ProbabilitySampler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/**
- * Utility class for tracing within Accumulo. Not intended for client use!
- */
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+
public class TraceUtil {
- private static final Logger log = LoggerFactory.getLogger(TraceUtil.class);
- public static final String TRACE_HOST_PROPERTY = "trace.host";
- public static final String TRACE_SERVICE_PROPERTY = "trace.service";
- public static final String TRACER_ZK_HOST = "tracer.zookeeper.host";
- public static final String TRACER_ZK_TIMEOUT = "tracer.zookeeper.timeout";
- public static final String TRACER_ZK_PATH = "tracer.zookeeper.path";
- private static final HashSet<SpanReceiver> receivers = new HashSet<>();
+ public static final Logger LOG = LoggerFactory.getLogger(TraceUtil.class);
+
+ private static final String SPAN_FORMAT = "%s::%s";
+
+ private static Tracer instance = null;
+ private static String name = null;
+ private static boolean tracing = false;
+
+ private static void initializeInternals(OpenTelemetry ot, String
instrumentationName) {
+ // TODO: Is there a way to get our version to pass to getTracer() ?
+ name = instrumentationName;
+ instance = ot.getTracer(name);
+ tracing = (!ot.equals(OpenTelemetry.noop()));
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance:
{}", tracing,
+ ot.getClass(), instance.getClass());
+ }
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
- * Properties required in the client configuration include
- * {@link org.apache.accumulo.core.conf.ClientProperty#TRACE_SPAN_RECEIVERS}
and any properties
- * specific to the span receiver.
+ * Initialize TracerUtil using the OpenTelemetry parameter and
instrumentationName
+ *
+ * @param ot
+ * OpenTelemetry instance
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
*/
- public static void enableClientTraces(String hostname, String service,
Properties properties) {
- // @formatter:off
- enableTracing(hostname, service,
- ClientProperty.TRACE_SPAN_RECEIVERS.getValue(properties),
- ClientProperty.INSTANCE_ZOOKEEPERS.getValue(properties),
- ConfigurationTypeHelper
-
.getTimeInMillis(ClientProperty.INSTANCE_ZOOKEEPERS_TIMEOUT.getValue(properties)),
- ClientProperty.TRACE_ZOOKEEPER_PATH.getValue(properties),
- ClientProperty.toMap(
- ClientProperty.getPrefix(properties,
ClientProperty.TRACE_SPAN_RECEIVER_PREFIX)));
- // @formatter:on
+ public static void initializeTracer(OpenTelemetry ot, String
instrumentationName) {
+ if (instance != null) {
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
+ }
}
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
+ * Use the property values in the AccumuloConfiguration to call
+ * {@link #initializeTracer(boolean, String, String)}
+ *
+ * @param conf
+ * AccumuloConfiguration
+ * @throws Exception
+ * unable to find or load class
*/
- public static void enableServerTraces(String hostname, String service,
- AccumuloConfiguration conf) {
- // @formatter:off
- enableTracing(hostname, service,
- conf.get(Property.TRACE_SPAN_RECEIVERS),
- conf.get(Property.INSTANCE_ZK_HOST),
- conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
- conf.get(Property.TRACE_ZK_PATH),
-
conf.getAllPropertiesWithPrefix(Property.TRACE_SPAN_RECEIVER_PREFIX));
- // @formatter:on
+ public static void initializeTracer(AccumuloConfiguration conf) throws
Exception {
+ initializeTracer(conf.getBoolean(Property.GENERAL_OPENTELEMETRY_ENABLED),
+ conf.get(Property.GENERAL_OPENTELEMETRY_FACTORY),
+ conf.get(Property.GENERAL_OPENTELEMETRY_NAME));
}
- private static void enableTracing(String hostname, String service, String
spanReceivers,
- String zookeepers, long timeout, String zkPath, Map<String,String>
spanReceiverProps) {
-
- Map<String,
- String> htraceConfigProps =
spanReceiverProps.entrySet().stream().collect(Collectors.toMap(
- k ->
String.valueOf(k).substring(Property.TRACE_SPAN_RECEIVER_PREFIX.getKey().length()),
- String::valueOf, (a, b) -> {
- throw new AssertionError("duplicate can't happen");
- }, HashMap::new));
- htraceConfigProps.put(TRACER_ZK_HOST, zookeepers);
- htraceConfigProps.put(TRACER_ZK_TIMEOUT, Long.toString(timeout));
- htraceConfigProps.put(TRACER_ZK_PATH, zkPath);
- if (hostname != null) {
- htraceConfigProps.put(TRACE_HOST_PROPERTY, hostname);
- }
- if (service != null) {
- htraceConfigProps.put(TRACE_SERVICE_PROPERTY, service);
- }
- ShutdownHookManager.get().addShutdownHook(TraceUtil::disable, 0);
- synchronized (receivers) {
- if (!receivers.isEmpty()) {
- log.info("Already loaded span receivers, enable tracing does not need
to be called again");
- return;
- }
- if (spanReceivers == null) {
- return;
- }
- Stream.of(spanReceivers.split(",")).map(String::trim).filter(s ->
!s.isEmpty())
- .forEach(className -> {
- HTraceConfiguration htraceConf =
HTraceConfiguration.fromMap(htraceConfigProps);
- SpanReceiverBuilder builder = new SpanReceiverBuilder(htraceConf);
- SpanReceiver rcvr =
builder.spanReceiverClass(className.trim()).build();
- if (rcvr == null) {
- log.warn("Failed to load SpanReceiver {}", className);
- } else {
- receivers.add(rcvr);
- log.debug("SpanReceiver {} was loaded successfully.", className);
- }
- });
- for (SpanReceiver rcvr : receivers) {
- Trace.addReceiver(rcvr);
+ /**
+ * If not enabled, the OpenTelemetry implementation will be set to the NoOp
implementation. If
+ * enabled and a factoryClass is supplied, then we will get the
OpenTelemetry instance from the
+ * factory class.
+ *
+ * @param enabled
+ * whether or not tracing is enabled
+ * @param factoryClass
+ * name of class to load
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
+ * @throws Exception
+ * unable to find or load class
+ */
+ public static void initializeTracer(boolean enabled, String factoryClass,
+ String instrumentationName) throws Exception {
+ if (instance == null) {
+ OpenTelemetry ot = null;
+ if (!enabled) {
+ ot = OpenTelemetry.noop();
+ } else if (factoryClass != null && !factoryClass.isEmpty()) {
+ Class<? extends OpenTelemetryFactory> clazz =
+ (Class<? extends OpenTelemetryFactory>)
ClassLoaderUtil.loadClass(factoryClass,
+ OpenTelemetryFactory.class);
+ OpenTelemetryFactory factory =
clazz.getDeclaredConstructor().newInstance();
+ ot = factory.getOpenTelemetry();
+ LOG.info("OpenTelemetry configured and set from {}", clazz);
+ } else {
+ ot = GlobalOpenTelemetry.get();
}
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
}
}
/**
- * Disable tracing by closing SpanReceivers for the current process.
+ * @return the Tracer set on the GlobalOpenTelemetry object
*/
- public static void disable() {
- synchronized (receivers) {
- receivers.forEach(rcvr -> {
- try {
- rcvr.close();
- } catch (IOException e) {
- log.warn("Unable to close SpanReceiver correctly: {}",
e.getMessage(), e);
- }
- });
- receivers.clear();
+ private static Tracer getTracer() {
+ if (Objects.isNull(instance)) {
+ LOG.warn("initializeTracer not called, using
GlobalOpenTelemetry.getTracer()");
+ instance = GlobalOpenTelemetry.getTracer(name);
+ tracing = (!instance.equals(OpenTelemetry.noop().getTracer(name)));
+ LOG.info("Trace enabled: {}, Tracer is: {}", tracing,
instance.getClass());
}
+ return instance;
}
/**
- * Continue a trace by starting a new span with a given parent and
description.
+ * @return true if an OpenTelemetry Tracer implementation has been set,
false if the NoOp Tracer
+ * is being used.
*/
- public static TraceScope trace(TInfo info, String description) {
- return info.traceId == 0 ? NullScope.INSTANCE
- : Trace.startSpan(description, new TraceInfo(info.traceId,
info.parentId));
+ public static boolean isTracing() {
+ return tracing;
}
- private static final TInfo DONT_TRACE = new TInfo(0, 0);
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind) {
+ return createSpan(caller, spanName, kind, null, false, null);
+ }
- /**
- * Obtain {@link org.apache.accumulo.core.trace.thrift.TInfo} for the
current span.
- */
- public static TInfo traceInfo() {
- Span span = Trace.currentSpan();
- if (span != null) {
- return new TInfo(span.getTraceId(), span.getSpanId());
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind, Context parent) {
+ return createSpan(caller, spanName, kind, null, false, parent);
+ }
+
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind,
+ Map<String,String> attributes) {
+ return createSpan(caller, spanName, kind, attributes, false, null);
+ }
+
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind,
+ Map<String,String> attributes, boolean setNoParent, Context parent) {
+ final String name = String.format(SPAN_FORMAT, caller.getSimpleName(),
spanName);
+ final SpanBuilder builder = getTracer().spanBuilder(name);
+ builder.setSpanKind(kind);
+ if (attributes != null) {
+ attributes.forEach((k, v) -> builder.setAttribute(k, v));
Review comment:
```suggestion
attributes.forEach(builder::setAttribute);
```
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -18,211 +18,230 @@
*/
package org.apache.accumulo.core.trace;
-import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
import java.util.Map;
-import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.Objects;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
-import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.spi.trace.OpenTelemetryFactory;
import org.apache.accumulo.core.trace.thrift.TInfo;
-import org.apache.hadoop.util.ShutdownHookManager;
-import org.apache.htrace.HTraceConfiguration;
-import org.apache.htrace.NullScope;
-import org.apache.htrace.Span;
-import org.apache.htrace.SpanReceiver;
-import org.apache.htrace.SpanReceiverBuilder;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceInfo;
-import org.apache.htrace.TraceScope;
-import org.apache.htrace.impl.CountSampler;
-import org.apache.htrace.impl.ProbabilitySampler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/**
- * Utility class for tracing within Accumulo. Not intended for client use!
- */
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+
public class TraceUtil {
- private static final Logger log = LoggerFactory.getLogger(TraceUtil.class);
- public static final String TRACE_HOST_PROPERTY = "trace.host";
- public static final String TRACE_SERVICE_PROPERTY = "trace.service";
- public static final String TRACER_ZK_HOST = "tracer.zookeeper.host";
- public static final String TRACER_ZK_TIMEOUT = "tracer.zookeeper.timeout";
- public static final String TRACER_ZK_PATH = "tracer.zookeeper.path";
- private static final HashSet<SpanReceiver> receivers = new HashSet<>();
+ public static final Logger LOG = LoggerFactory.getLogger(TraceUtil.class);
+
+ private static final String SPAN_FORMAT = "%s::%s";
+
+ private static Tracer instance = null;
+ private static String name = null;
+ private static boolean tracing = false;
+
+ private static void initializeInternals(OpenTelemetry ot, String
instrumentationName) {
+ // TODO: Is there a way to get our version to pass to getTracer() ?
+ name = instrumentationName;
+ instance = ot.getTracer(name);
+ tracing = (!ot.equals(OpenTelemetry.noop()));
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance:
{}", tracing,
+ ot.getClass(), instance.getClass());
+ }
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
- * Properties required in the client configuration include
- * {@link org.apache.accumulo.core.conf.ClientProperty#TRACE_SPAN_RECEIVERS}
and any properties
- * specific to the span receiver.
+ * Initialize TracerUtil using the OpenTelemetry parameter and
instrumentationName
+ *
+ * @param ot
+ * OpenTelemetry instance
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
*/
- public static void enableClientTraces(String hostname, String service,
Properties properties) {
- // @formatter:off
- enableTracing(hostname, service,
- ClientProperty.TRACE_SPAN_RECEIVERS.getValue(properties),
- ClientProperty.INSTANCE_ZOOKEEPERS.getValue(properties),
- ConfigurationTypeHelper
-
.getTimeInMillis(ClientProperty.INSTANCE_ZOOKEEPERS_TIMEOUT.getValue(properties)),
- ClientProperty.TRACE_ZOOKEEPER_PATH.getValue(properties),
- ClientProperty.toMap(
- ClientProperty.getPrefix(properties,
ClientProperty.TRACE_SPAN_RECEIVER_PREFIX)));
- // @formatter:on
+ public static void initializeTracer(OpenTelemetry ot, String
instrumentationName) {
+ if (instance != null) {
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
+ }
}
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
+ * Use the property values in the AccumuloConfiguration to call
+ * {@link #initializeTracer(boolean, String, String)}
+ *
+ * @param conf
+ * AccumuloConfiguration
+ * @throws Exception
+ * unable to find or load class
*/
- public static void enableServerTraces(String hostname, String service,
- AccumuloConfiguration conf) {
- // @formatter:off
- enableTracing(hostname, service,
- conf.get(Property.TRACE_SPAN_RECEIVERS),
- conf.get(Property.INSTANCE_ZK_HOST),
- conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
- conf.get(Property.TRACE_ZK_PATH),
-
conf.getAllPropertiesWithPrefix(Property.TRACE_SPAN_RECEIVER_PREFIX));
- // @formatter:on
+ public static void initializeTracer(AccumuloConfiguration conf) throws
Exception {
+ initializeTracer(conf.getBoolean(Property.GENERAL_OPENTELEMETRY_ENABLED),
+ conf.get(Property.GENERAL_OPENTELEMETRY_FACTORY),
+ conf.get(Property.GENERAL_OPENTELEMETRY_NAME));
}
- private static void enableTracing(String hostname, String service, String
spanReceivers,
- String zookeepers, long timeout, String zkPath, Map<String,String>
spanReceiverProps) {
-
- Map<String,
- String> htraceConfigProps =
spanReceiverProps.entrySet().stream().collect(Collectors.toMap(
- k ->
String.valueOf(k).substring(Property.TRACE_SPAN_RECEIVER_PREFIX.getKey().length()),
- String::valueOf, (a, b) -> {
- throw new AssertionError("duplicate can't happen");
- }, HashMap::new));
- htraceConfigProps.put(TRACER_ZK_HOST, zookeepers);
- htraceConfigProps.put(TRACER_ZK_TIMEOUT, Long.toString(timeout));
- htraceConfigProps.put(TRACER_ZK_PATH, zkPath);
- if (hostname != null) {
- htraceConfigProps.put(TRACE_HOST_PROPERTY, hostname);
- }
- if (service != null) {
- htraceConfigProps.put(TRACE_SERVICE_PROPERTY, service);
- }
- ShutdownHookManager.get().addShutdownHook(TraceUtil::disable, 0);
- synchronized (receivers) {
- if (!receivers.isEmpty()) {
- log.info("Already loaded span receivers, enable tracing does not need
to be called again");
- return;
- }
- if (spanReceivers == null) {
- return;
- }
- Stream.of(spanReceivers.split(",")).map(String::trim).filter(s ->
!s.isEmpty())
- .forEach(className -> {
- HTraceConfiguration htraceConf =
HTraceConfiguration.fromMap(htraceConfigProps);
- SpanReceiverBuilder builder = new SpanReceiverBuilder(htraceConf);
- SpanReceiver rcvr =
builder.spanReceiverClass(className.trim()).build();
- if (rcvr == null) {
- log.warn("Failed to load SpanReceiver {}", className);
- } else {
- receivers.add(rcvr);
- log.debug("SpanReceiver {} was loaded successfully.", className);
- }
- });
- for (SpanReceiver rcvr : receivers) {
- Trace.addReceiver(rcvr);
+ /**
+ * If not enabled, the OpenTelemetry implementation will be set to the NoOp
implementation. If
+ * enabled and a factoryClass is supplied, then we will get the
OpenTelemetry instance from the
+ * factory class.
+ *
+ * @param enabled
+ * whether or not tracing is enabled
+ * @param factoryClass
+ * name of class to load
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
+ * @throws Exception
+ * unable to find or load class
+ */
+ public static void initializeTracer(boolean enabled, String factoryClass,
+ String instrumentationName) throws Exception {
+ if (instance == null) {
+ OpenTelemetry ot = null;
+ if (!enabled) {
+ ot = OpenTelemetry.noop();
+ } else if (factoryClass != null && !factoryClass.isEmpty()) {
+ Class<? extends OpenTelemetryFactory> clazz =
+ (Class<? extends OpenTelemetryFactory>)
ClassLoaderUtil.loadClass(factoryClass,
+ OpenTelemetryFactory.class);
+ OpenTelemetryFactory factory =
clazz.getDeclaredConstructor().newInstance();
+ ot = factory.getOpenTelemetry();
+ LOG.info("OpenTelemetry configured and set from {}", clazz);
+ } else {
+ ot = GlobalOpenTelemetry.get();
}
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
}
}
/**
- * Disable tracing by closing SpanReceivers for the current process.
+ * @return the Tracer set on the GlobalOpenTelemetry object
*/
- public static void disable() {
- synchronized (receivers) {
- receivers.forEach(rcvr -> {
- try {
- rcvr.close();
- } catch (IOException e) {
- log.warn("Unable to close SpanReceiver correctly: {}",
e.getMessage(), e);
- }
- });
- receivers.clear();
+ private static Tracer getTracer() {
+ if (Objects.isNull(instance)) {
+ LOG.warn("initializeTracer not called, using
GlobalOpenTelemetry.getTracer()");
+ instance = GlobalOpenTelemetry.getTracer(name);
+ tracing = (!instance.equals(OpenTelemetry.noop().getTracer(name)));
Review comment:
Extra parens makes it harder to parse visually
```suggestion
tracing = !instance.equals(OpenTelemetry.noop().getTracer(name));
```
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -18,211 +18,230 @@
*/
package org.apache.accumulo.core.trace;
-import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
import java.util.Map;
-import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.Objects;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
-import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.spi.trace.OpenTelemetryFactory;
import org.apache.accumulo.core.trace.thrift.TInfo;
-import org.apache.hadoop.util.ShutdownHookManager;
-import org.apache.htrace.HTraceConfiguration;
-import org.apache.htrace.NullScope;
-import org.apache.htrace.Span;
-import org.apache.htrace.SpanReceiver;
-import org.apache.htrace.SpanReceiverBuilder;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceInfo;
-import org.apache.htrace.TraceScope;
-import org.apache.htrace.impl.CountSampler;
-import org.apache.htrace.impl.ProbabilitySampler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/**
- * Utility class for tracing within Accumulo. Not intended for client use!
- */
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+
public class TraceUtil {
- private static final Logger log = LoggerFactory.getLogger(TraceUtil.class);
- public static final String TRACE_HOST_PROPERTY = "trace.host";
- public static final String TRACE_SERVICE_PROPERTY = "trace.service";
- public static final String TRACER_ZK_HOST = "tracer.zookeeper.host";
- public static final String TRACER_ZK_TIMEOUT = "tracer.zookeeper.timeout";
- public static final String TRACER_ZK_PATH = "tracer.zookeeper.path";
- private static final HashSet<SpanReceiver> receivers = new HashSet<>();
+ public static final Logger LOG = LoggerFactory.getLogger(TraceUtil.class);
+
+ private static final String SPAN_FORMAT = "%s::%s";
+
+ private static Tracer instance = null;
+ private static String name = null;
+ private static boolean tracing = false;
+
+ private static void initializeInternals(OpenTelemetry ot, String
instrumentationName) {
+ // TODO: Is there a way to get our version to pass to getTracer() ?
+ name = instrumentationName;
+ instance = ot.getTracer(name);
+ tracing = (!ot.equals(OpenTelemetry.noop()));
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance:
{}", tracing,
+ ot.getClass(), instance.getClass());
+ }
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
- * Properties required in the client configuration include
- * {@link org.apache.accumulo.core.conf.ClientProperty#TRACE_SPAN_RECEIVERS}
and any properties
- * specific to the span receiver.
+ * Initialize TracerUtil using the OpenTelemetry parameter and
instrumentationName
+ *
+ * @param ot
+ * OpenTelemetry instance
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
*/
- public static void enableClientTraces(String hostname, String service,
Properties properties) {
- // @formatter:off
- enableTracing(hostname, service,
- ClientProperty.TRACE_SPAN_RECEIVERS.getValue(properties),
- ClientProperty.INSTANCE_ZOOKEEPERS.getValue(properties),
- ConfigurationTypeHelper
-
.getTimeInMillis(ClientProperty.INSTANCE_ZOOKEEPERS_TIMEOUT.getValue(properties)),
- ClientProperty.TRACE_ZOOKEEPER_PATH.getValue(properties),
- ClientProperty.toMap(
- ClientProperty.getPrefix(properties,
ClientProperty.TRACE_SPAN_RECEIVER_PREFIX)));
- // @formatter:on
+ public static void initializeTracer(OpenTelemetry ot, String
instrumentationName) {
+ if (instance != null) {
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
+ }
}
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
+ * Use the property values in the AccumuloConfiguration to call
+ * {@link #initializeTracer(boolean, String, String)}
+ *
+ * @param conf
+ * AccumuloConfiguration
+ * @throws Exception
+ * unable to find or load class
*/
- public static void enableServerTraces(String hostname, String service,
- AccumuloConfiguration conf) {
- // @formatter:off
- enableTracing(hostname, service,
- conf.get(Property.TRACE_SPAN_RECEIVERS),
- conf.get(Property.INSTANCE_ZK_HOST),
- conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
- conf.get(Property.TRACE_ZK_PATH),
-
conf.getAllPropertiesWithPrefix(Property.TRACE_SPAN_RECEIVER_PREFIX));
- // @formatter:on
+ public static void initializeTracer(AccumuloConfiguration conf) throws
Exception {
+ initializeTracer(conf.getBoolean(Property.GENERAL_OPENTELEMETRY_ENABLED),
+ conf.get(Property.GENERAL_OPENTELEMETRY_FACTORY),
+ conf.get(Property.GENERAL_OPENTELEMETRY_NAME));
}
- private static void enableTracing(String hostname, String service, String
spanReceivers,
- String zookeepers, long timeout, String zkPath, Map<String,String>
spanReceiverProps) {
-
- Map<String,
- String> htraceConfigProps =
spanReceiverProps.entrySet().stream().collect(Collectors.toMap(
- k ->
String.valueOf(k).substring(Property.TRACE_SPAN_RECEIVER_PREFIX.getKey().length()),
- String::valueOf, (a, b) -> {
- throw new AssertionError("duplicate can't happen");
- }, HashMap::new));
- htraceConfigProps.put(TRACER_ZK_HOST, zookeepers);
- htraceConfigProps.put(TRACER_ZK_TIMEOUT, Long.toString(timeout));
- htraceConfigProps.put(TRACER_ZK_PATH, zkPath);
- if (hostname != null) {
- htraceConfigProps.put(TRACE_HOST_PROPERTY, hostname);
- }
- if (service != null) {
- htraceConfigProps.put(TRACE_SERVICE_PROPERTY, service);
- }
- ShutdownHookManager.get().addShutdownHook(TraceUtil::disable, 0);
- synchronized (receivers) {
- if (!receivers.isEmpty()) {
- log.info("Already loaded span receivers, enable tracing does not need
to be called again");
- return;
- }
- if (spanReceivers == null) {
- return;
- }
- Stream.of(spanReceivers.split(",")).map(String::trim).filter(s ->
!s.isEmpty())
- .forEach(className -> {
- HTraceConfiguration htraceConf =
HTraceConfiguration.fromMap(htraceConfigProps);
- SpanReceiverBuilder builder = new SpanReceiverBuilder(htraceConf);
- SpanReceiver rcvr =
builder.spanReceiverClass(className.trim()).build();
- if (rcvr == null) {
- log.warn("Failed to load SpanReceiver {}", className);
- } else {
- receivers.add(rcvr);
- log.debug("SpanReceiver {} was loaded successfully.", className);
- }
- });
- for (SpanReceiver rcvr : receivers) {
- Trace.addReceiver(rcvr);
+ /**
+ * If not enabled, the OpenTelemetry implementation will be set to the NoOp
implementation. If
+ * enabled and a factoryClass is supplied, then we will get the
OpenTelemetry instance from the
+ * factory class.
+ *
+ * @param enabled
+ * whether or not tracing is enabled
+ * @param factoryClass
+ * name of class to load
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
+ * @throws Exception
+ * unable to find or load class
+ */
+ public static void initializeTracer(boolean enabled, String factoryClass,
+ String instrumentationName) throws Exception {
+ if (instance == null) {
+ OpenTelemetry ot = null;
+ if (!enabled) {
+ ot = OpenTelemetry.noop();
+ } else if (factoryClass != null && !factoryClass.isEmpty()) {
+ Class<? extends OpenTelemetryFactory> clazz =
+ (Class<? extends OpenTelemetryFactory>)
ClassLoaderUtil.loadClass(factoryClass,
+ OpenTelemetryFactory.class);
+ OpenTelemetryFactory factory =
clazz.getDeclaredConstructor().newInstance();
+ ot = factory.getOpenTelemetry();
+ LOG.info("OpenTelemetry configured and set from {}", clazz);
+ } else {
+ ot = GlobalOpenTelemetry.get();
}
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
}
}
/**
- * Disable tracing by closing SpanReceivers for the current process.
+ * @return the Tracer set on the GlobalOpenTelemetry object
*/
- public static void disable() {
- synchronized (receivers) {
- receivers.forEach(rcvr -> {
- try {
- rcvr.close();
- } catch (IOException e) {
- log.warn("Unable to close SpanReceiver correctly: {}",
e.getMessage(), e);
- }
- });
- receivers.clear();
+ private static Tracer getTracer() {
+ if (Objects.isNull(instance)) {
+ LOG.warn("initializeTracer not called, using
GlobalOpenTelemetry.getTracer()");
+ instance = GlobalOpenTelemetry.getTracer(name);
+ tracing = (!instance.equals(OpenTelemetry.noop().getTracer(name)));
+ LOG.info("Trace enabled: {}, Tracer is: {}", tracing,
instance.getClass());
}
+ return instance;
}
/**
- * Continue a trace by starting a new span with a given parent and
description.
+ * @return true if an OpenTelemetry Tracer implementation has been set,
false if the NoOp Tracer
+ * is being used.
*/
- public static TraceScope trace(TInfo info, String description) {
- return info.traceId == 0 ? NullScope.INSTANCE
- : Trace.startSpan(description, new TraceInfo(info.traceId,
info.parentId));
+ public static boolean isTracing() {
+ return tracing;
}
- private static final TInfo DONT_TRACE = new TInfo(0, 0);
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind) {
+ return createSpan(caller, spanName, kind, null, false, null);
+ }
- /**
- * Obtain {@link org.apache.accumulo.core.trace.thrift.TInfo} for the
current span.
- */
- public static TInfo traceInfo() {
- Span span = Trace.currentSpan();
- if (span != null) {
- return new TInfo(span.getTraceId(), span.getSpanId());
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind, Context parent) {
+ return createSpan(caller, spanName, kind, null, false, parent);
+ }
+
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind,
+ Map<String,String> attributes) {
+ return createSpan(caller, spanName, kind, attributes, false, null);
+ }
+
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind,
+ Map<String,String> attributes, boolean setNoParent, Context parent) {
Review comment:
`setNoParent` and `parent` seem to be performing an overlapping role.
Can `setNoParent` always be inferred from `parent` being `null`?
##########
File path:
core/src/test/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParserTest.java
##########
@@ -75,13 +74,12 @@ public void testParseWithExternalCompactions() throws
Exception {
Map<String,String> contents =
ClusterConfigParser.parseConfiguration(new
File(configFile.toURI()).getAbsolutePath());
- assertEquals(9, contents.size());
+ assertEquals(8, contents.size());
assertTrue(contents.containsKey("manager"));
assertEquals("localhost1 localhost2", contents.get("manager"));
assertTrue(contents.containsKey("monitor"));
assertEquals("localhost1 localhost2", contents.get("monitor"));
- assertTrue(contents.containsKey("tracer"));
- assertEquals("localhost", contents.get("tracer"));
+ assertFalse(contents.containsKey("tracer"));
Review comment:
```suggestion
```
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -231,8 +250,16 @@ public static ProbabilitySampler probabilitySampler(double
fraction) {
if (args == null || args.length < 1 || args[0] == null || !(args[0]
instanceof TInfo)) {
return method.invoke(instance, args);
}
- try (TraceScope span = trace((TInfo) args[0], method.getName())) {
+ TInfo tinfo = (TInfo) args[0];
+ getContext(tinfo).makeCurrent();
+ Span span = createSpan(instance.getClass(), method.getName(),
SpanKind.SERVER);
+ try (Scope scope = span.makeCurrent()) {
Review comment:
I'm not sure this is the right pattern. I don't think we want to
`makeCurrent` on the parent, because the Closeable resource object it returns
is never closed. Instead, I think we just want to set the parent on the Span we
create:
```suggestion
Context parent = getContext((TInfo) args[0]);
Span span = createSpan(instance.getClass(), method.getName(),
SpanKind.SERVER, parent);
try (Scope scope = span.makeCurrent()) {
```
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -18,211 +18,230 @@
*/
package org.apache.accumulo.core.trace;
-import java.io.IOException;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
import java.util.Map;
-import java.util.Properties;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
+import java.util.Objects;
+import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
-import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.spi.trace.OpenTelemetryFactory;
import org.apache.accumulo.core.trace.thrift.TInfo;
-import org.apache.hadoop.util.ShutdownHookManager;
-import org.apache.htrace.HTraceConfiguration;
-import org.apache.htrace.NullScope;
-import org.apache.htrace.Span;
-import org.apache.htrace.SpanReceiver;
-import org.apache.htrace.SpanReceiverBuilder;
-import org.apache.htrace.Trace;
-import org.apache.htrace.TraceInfo;
-import org.apache.htrace.TraceScope;
-import org.apache.htrace.impl.CountSampler;
-import org.apache.htrace.impl.ProbabilitySampler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-/**
- * Utility class for tracing within Accumulo. Not intended for client use!
- */
+import io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+
public class TraceUtil {
- private static final Logger log = LoggerFactory.getLogger(TraceUtil.class);
- public static final String TRACE_HOST_PROPERTY = "trace.host";
- public static final String TRACE_SERVICE_PROPERTY = "trace.service";
- public static final String TRACER_ZK_HOST = "tracer.zookeeper.host";
- public static final String TRACER_ZK_TIMEOUT = "tracer.zookeeper.timeout";
- public static final String TRACER_ZK_PATH = "tracer.zookeeper.path";
- private static final HashSet<SpanReceiver> receivers = new HashSet<>();
+ public static final Logger LOG = LoggerFactory.getLogger(TraceUtil.class);
+
+ private static final String SPAN_FORMAT = "%s::%s";
+
+ private static Tracer instance = null;
+ private static String name = null;
+ private static boolean tracing = false;
+
+ private static void initializeInternals(OpenTelemetry ot, String
instrumentationName) {
+ // TODO: Is there a way to get our version to pass to getTracer() ?
+ name = instrumentationName;
+ instance = ot.getTracer(name);
+ tracing = (!ot.equals(OpenTelemetry.noop()));
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance:
{}", tracing,
+ ot.getClass(), instance.getClass());
+ }
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
- * Properties required in the client configuration include
- * {@link org.apache.accumulo.core.conf.ClientProperty#TRACE_SPAN_RECEIVERS}
and any properties
- * specific to the span receiver.
+ * Initialize TracerUtil using the OpenTelemetry parameter and
instrumentationName
+ *
+ * @param ot
+ * OpenTelemetry instance
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
*/
- public static void enableClientTraces(String hostname, String service,
Properties properties) {
- // @formatter:off
- enableTracing(hostname, service,
- ClientProperty.TRACE_SPAN_RECEIVERS.getValue(properties),
- ClientProperty.INSTANCE_ZOOKEEPERS.getValue(properties),
- ConfigurationTypeHelper
-
.getTimeInMillis(ClientProperty.INSTANCE_ZOOKEEPERS_TIMEOUT.getValue(properties)),
- ClientProperty.TRACE_ZOOKEEPER_PATH.getValue(properties),
- ClientProperty.toMap(
- ClientProperty.getPrefix(properties,
ClientProperty.TRACE_SPAN_RECEIVER_PREFIX)));
- // @formatter:on
+ public static void initializeTracer(OpenTelemetry ot, String
instrumentationName) {
+ if (instance != null) {
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
+ }
}
/**
- * Enable tracing by setting up SpanReceivers for the current process. If
host name is null, it
- * will be determined. If service name is null, the simple name of the class
will be used.
+ * Use the property values in the AccumuloConfiguration to call
+ * {@link #initializeTracer(boolean, String, String)}
+ *
+ * @param conf
+ * AccumuloConfiguration
+ * @throws Exception
+ * unable to find or load class
*/
- public static void enableServerTraces(String hostname, String service,
- AccumuloConfiguration conf) {
- // @formatter:off
- enableTracing(hostname, service,
- conf.get(Property.TRACE_SPAN_RECEIVERS),
- conf.get(Property.INSTANCE_ZK_HOST),
- conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
- conf.get(Property.TRACE_ZK_PATH),
-
conf.getAllPropertiesWithPrefix(Property.TRACE_SPAN_RECEIVER_PREFIX));
- // @formatter:on
+ public static void initializeTracer(AccumuloConfiguration conf) throws
Exception {
+ initializeTracer(conf.getBoolean(Property.GENERAL_OPENTELEMETRY_ENABLED),
+ conf.get(Property.GENERAL_OPENTELEMETRY_FACTORY),
+ conf.get(Property.GENERAL_OPENTELEMETRY_NAME));
}
- private static void enableTracing(String hostname, String service, String
spanReceivers,
- String zookeepers, long timeout, String zkPath, Map<String,String>
spanReceiverProps) {
-
- Map<String,
- String> htraceConfigProps =
spanReceiverProps.entrySet().stream().collect(Collectors.toMap(
- k ->
String.valueOf(k).substring(Property.TRACE_SPAN_RECEIVER_PREFIX.getKey().length()),
- String::valueOf, (a, b) -> {
- throw new AssertionError("duplicate can't happen");
- }, HashMap::new));
- htraceConfigProps.put(TRACER_ZK_HOST, zookeepers);
- htraceConfigProps.put(TRACER_ZK_TIMEOUT, Long.toString(timeout));
- htraceConfigProps.put(TRACER_ZK_PATH, zkPath);
- if (hostname != null) {
- htraceConfigProps.put(TRACE_HOST_PROPERTY, hostname);
- }
- if (service != null) {
- htraceConfigProps.put(TRACE_SERVICE_PROPERTY, service);
- }
- ShutdownHookManager.get().addShutdownHook(TraceUtil::disable, 0);
- synchronized (receivers) {
- if (!receivers.isEmpty()) {
- log.info("Already loaded span receivers, enable tracing does not need
to be called again");
- return;
- }
- if (spanReceivers == null) {
- return;
- }
- Stream.of(spanReceivers.split(",")).map(String::trim).filter(s ->
!s.isEmpty())
- .forEach(className -> {
- HTraceConfiguration htraceConf =
HTraceConfiguration.fromMap(htraceConfigProps);
- SpanReceiverBuilder builder = new SpanReceiverBuilder(htraceConf);
- SpanReceiver rcvr =
builder.spanReceiverClass(className.trim()).build();
- if (rcvr == null) {
- log.warn("Failed to load SpanReceiver {}", className);
- } else {
- receivers.add(rcvr);
- log.debug("SpanReceiver {} was loaded successfully.", className);
- }
- });
- for (SpanReceiver rcvr : receivers) {
- Trace.addReceiver(rcvr);
+ /**
+ * If not enabled, the OpenTelemetry implementation will be set to the NoOp
implementation. If
+ * enabled and a factoryClass is supplied, then we will get the
OpenTelemetry instance from the
+ * factory class.
+ *
+ * @param enabled
+ * whether or not tracing is enabled
+ * @param factoryClass
+ * name of class to load
+ * @param instrumentationName
+ * OpenTelemetry instrumentation library name
+ * @throws Exception
+ * unable to find or load class
+ */
+ public static void initializeTracer(boolean enabled, String factoryClass,
+ String instrumentationName) throws Exception {
+ if (instance == null) {
+ OpenTelemetry ot = null;
+ if (!enabled) {
+ ot = OpenTelemetry.noop();
+ } else if (factoryClass != null && !factoryClass.isEmpty()) {
+ Class<? extends OpenTelemetryFactory> clazz =
+ (Class<? extends OpenTelemetryFactory>)
ClassLoaderUtil.loadClass(factoryClass,
+ OpenTelemetryFactory.class);
+ OpenTelemetryFactory factory =
clazz.getDeclaredConstructor().newInstance();
+ ot = factory.getOpenTelemetry();
+ LOG.info("OpenTelemetry configured and set from {}", clazz);
+ } else {
+ ot = GlobalOpenTelemetry.get();
}
+ initializeInternals(ot, instrumentationName);
+ } else {
+ LOG.warn("Tracer already initialized.");
}
}
/**
- * Disable tracing by closing SpanReceivers for the current process.
+ * @return the Tracer set on the GlobalOpenTelemetry object
*/
- public static void disable() {
- synchronized (receivers) {
- receivers.forEach(rcvr -> {
- try {
- rcvr.close();
- } catch (IOException e) {
- log.warn("Unable to close SpanReceiver correctly: {}",
e.getMessage(), e);
- }
- });
- receivers.clear();
+ private static Tracer getTracer() {
+ if (Objects.isNull(instance)) {
+ LOG.warn("initializeTracer not called, using
GlobalOpenTelemetry.getTracer()");
+ instance = GlobalOpenTelemetry.getTracer(name);
+ tracing = (!instance.equals(OpenTelemetry.noop().getTracer(name)));
+ LOG.info("Trace enabled: {}, Tracer is: {}", tracing,
instance.getClass());
}
+ return instance;
}
/**
- * Continue a trace by starting a new span with a given parent and
description.
+ * @return true if an OpenTelemetry Tracer implementation has been set,
false if the NoOp Tracer
+ * is being used.
*/
- public static TraceScope trace(TInfo info, String description) {
- return info.traceId == 0 ? NullScope.INSTANCE
- : Trace.startSpan(description, new TraceInfo(info.traceId,
info.parentId));
+ public static boolean isTracing() {
+ return tracing;
}
- private static final TInfo DONT_TRACE = new TInfo(0, 0);
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind) {
+ return createSpan(caller, spanName, kind, null, false, null);
+ }
- /**
- * Obtain {@link org.apache.accumulo.core.trace.thrift.TInfo} for the
current span.
- */
- public static TInfo traceInfo() {
- Span span = Trace.currentSpan();
- if (span != null) {
- return new TInfo(span.getTraceId(), span.getSpanId());
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind, Context parent) {
+ return createSpan(caller, spanName, kind, null, false, parent);
+ }
+
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind,
+ Map<String,String> attributes) {
+ return createSpan(caller, spanName, kind, attributes, false, null);
+ }
+
+ public static Span createSpan(Class<?> caller, String spanName, SpanKind
kind,
+ Map<String,String> attributes, boolean setNoParent, Context parent) {
+ final String name = String.format(SPAN_FORMAT, caller.getSimpleName(),
spanName);
+ final SpanBuilder builder = getTracer().spanBuilder(name);
+ builder.setSpanKind(kind);
+ if (attributes != null) {
+ attributes.forEach((k, v) -> builder.setAttribute(k, v));
}
- return DONT_TRACE;
+ if (setNoParent) {
+ builder.setNoParent();
+ } else if (parent != null) {
+ builder.setParent(parent);
+ }
+ return builder.startSpan();
}
- public static CountSampler countSampler(long frequency) {
- return new CountSampler(HTraceConfiguration.fromMap(Collections
- .singletonMap(CountSampler.SAMPLER_FREQUENCY_CONF_KEY,
Long.toString(frequency))));
+ /**
+ * Record that an Exception occurred in the code covered by a Span
+ *
+ * @param span
+ * the span
+ * @param e
+ * the exception
+ * @param rethrown
+ * whether the exception is subsequently re-thrown
+ */
+ public static void setException(Span span, Throwable e, boolean rethrown) {
+ if (tracing) {
+ span.setStatus(StatusCode.ERROR);
+ span.recordException(e,
+ Attributes.builder().put(SemanticAttributes.EXCEPTION_TYPE,
e.getClass().getName())
+ .put(SemanticAttributes.EXCEPTION_MESSAGE, e.getMessage())
+ .put(SemanticAttributes.EXCEPTION_ESCAPED, rethrown).build());
+ }
}
- public static ProbabilitySampler probabilitySampler(double fraction) {
- return new ProbabilitySampler(HTraceConfiguration.fromMap(Collections
- .singletonMap(ProbabilitySampler.SAMPLER_FRACTION_CONF_KEY,
Double.toString(fraction))));
+ /**
+ * Obtain {@link org.apache.accumulo.core.trace.thrift.TInfo} for the
current context. This is
+ * used to send the current trace information to a remote process
+ */
+ public static TInfo traceInfo() {
Review comment:
This method's name seems to understate the role it is performing. I
think a better name would be something like `currentTraceContextInfo`, but
that's a bit long.
##########
File path:
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java
##########
@@ -160,7 +162,9 @@ MiniAccumuloConfigImpl initialize() {
mergeProp(Property.GC_CYCLE_DELAY.getKey(), "4s");
mergeProp(Property.GC_CYCLE_START.getKey(), "0s");
mergePropWithRandomPort(Property.MANAGER_CLIENTPORT.getKey());
- mergePropWithRandomPort(Property.TRACE_PORT.getKey());
+ @SuppressWarnings("deprecation")
+ final String tracePort = Property.TRACE_PORT.getKey();
+ mergePropWithRandomPort(tracePort);
Review comment:
Unused property need not be written to the file.
```suggestion
```
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/util/ChangeSecret.java
##########
@@ -63,18 +67,21 @@
public static void main(String[] args) throws Exception {
var siteConfig = SiteConfiguration.auto();
var hadoopConf = new Configuration();
+
Opts opts = new Opts();
- List<String> argsList = new ArrayList<>(args.length + 2);
- argsList.add("--old");
- argsList.add("--new");
- argsList.addAll(Arrays.asList(args));
- try (TraceScope clientSpan =
- opts.parseArgsAndTrace(ChangeSecret.class.getName(),
argsList.toArray(new String[0]))) {
-
- ServerContext context = opts.getServerContext();
- try (var fs = context.getVolumeManager()) {
- ServerDirs serverDirs = new ServerDirs(siteConfig, hadoopConf);
- verifyHdfsWritePermission(serverDirs, fs);
+ ServerContext context = opts.getServerContext();
+ try (var fs = context.getVolumeManager()) {
+ ServerDirs serverDirs = new ServerDirs(siteConfig, hadoopConf);
+ verifyHdfsWritePermission(serverDirs, fs);
+
+ List<String> argsList = new ArrayList<>(args.length + 2);
+ argsList.add("--old");
+ argsList.add("--new");
+ argsList.addAll(Arrays.asList(args));
+
+ opts.parseArgs(ChangeSecret.class.getName(), args);
+ Span span = TraceUtil.createSpan(ChangeSecret.class, "main",
SpanKind.CLIENT);
+ try (Scope scope = span.makeCurrent()) {
Review comment:
It's not clear why this was moved inside here, instead of wrapping all
of it, including the `verifyHdfsWritePermission` call above.
##########
File path: pom.xml
##########
@@ -177,6 +175,13 @@
<type>pom</type>
<scope>import</scope>
</dependency>
+ <dependency>
+ <groupId>io.opentelemetry</groupId>
+ <artifactId>opentelemetry-bom</artifactId>
+ <version>1.6.0</version>
Review comment:
```suggestion
<version>1.7.1</version>
```
##########
File path:
server/base/src/main/java/org/apache/accumulo/server/util/CheckForMetadataProblems.java
##########
@@ -175,14 +179,20 @@ private static void
checkMetadataAndRootTableEntries(String tableNameToCheck, Se
public static void main(String[] args) throws Exception {
opts = new ServerUtilOpts();
- try (TraceScope clientSpan =
- opts.parseArgsAndTrace(CheckForMetadataProblems.class.getName(),
args)) {
+ opts.parseArgs(CheckForMetadataProblems.class.getName(), args);
+ Span span = TraceUtil.createSpan(CheckForMetadataProblems.class, "main",
SpanKind.CLIENT);
+ try (Scope scope = span.makeCurrent()) {
checkMetadataAndRootTableEntries(RootTable.NAME, opts);
System.out.println();
checkMetadataAndRootTableEntries(MetadataTable.NAME, opts);
if (sawProblems)
throw new RuntimeException();
+ } catch (Exception e) {
+ TraceUtil.setException(span, e, true);
+ throw e;
Review comment:
I really wanted to try to wrap Span and Scope into a single Closeable
wrapper object, so we don't have to explicitly call span.end(), and it would
make the diff smaller, but I now realize that wouldn't work. There needs to be
an object not auto-closed, that is still "open" so you can update it with the
exception-stuff. It wouldn't work my way, because `close()` is called on
resources prior to the catch blocks executing. My way would only work if we did
not want to set exceptions on the Span.
It may still be worth it to create wrapper objects in order to hide
OpenTelemetry types behind an abstraction, so we don't have OpenTelemetry
imports scattered throughout, and harder to upgrade or switch to another impl
if things change in future, but so long as we want to set attributes in a catch
clause, we'll need both a Span and a Scope type of some kind, even if they both
wrap their respective OpenTelemetry types.
##########
File path: pom.xml
##########
@@ -301,6 +306,11 @@
<artifactId>commons-logging</artifactId>
<version>1.2</version>
</dependency>
+ <dependency>
+ <groupId>io.opentelemetry</groupId>
+ <artifactId>opentelemetry-semconv</artifactId>
+ <version>1.6.0-alpha</version>
Review comment:
It's not clear what this dependency provides us, but it turns out
there's a separate BOM we should be using to manage all the dependency versions
for the alpha (unstable) modules.
```suggestion
<artifactId>opentelemetry-bom-alpha</artifactId>
<version>1.7.1-alpha</version>
<type>pom</type>
<scope>import</scope>
```
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -606,8 +612,10 @@
"1.8.0"),
TSERV_MINC_MAXCONCURRENT("tserver.compaction.minor.concurrent.max", "4",
PropertyType.COUNT,
"The maximum number of concurrent minor compactions for a tablet
server", "1.3.5"),
+ @Deprecated(since = "2.1.0", forRemoval = true)
TSERV_MAJC_TRACE_PERCENT("tserver.compaction.major.trace.percent", "0.1",
PropertyType.FRACTION,
Review comment:
I looked into setting samplers, but it looks like we have to use the SDK
for that to create our own, more fully robust, instrumentation. But that kinda
precludes using user-pluggable OpenTelemetry instances. Unfortunately, you
can't just set the sampler on the specific Tracer instance when you get it from
the OpenTelemetry instance. This seems like a downgrade from what HTrace made
available.
--
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]
