This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/main by this push:
new 58431fe Minor improvements to opentelemetry tooling (#2363)
58431fe is described below
commit 58431fe6c0d8a8be8b1864b6d175906245c6fdfb
Author: Christopher Tubbs <ctubb...@apache.org>
AuthorDate: Mon Nov 22 14:11:04 2021 -0500
Minor improvements to opentelemetry tooling (#2363)
* Remove client-side properties and server-side property to set
OpenTelemetry factory. This isn't needed because:
* For server-side, we only need to use the GlobalOpenTelemetry
instance, and that can be overridden by standard means in
OpenTelemetry (class path setup for autoconfigure instance or
javaagent to inject a custom impl)
* For client-side, we can assume it is on by default and the user can
set their own GlobalOpenTelemetry instance (or set it up with class
path for autoconfigure or use javaagent to inject)
* Remove no longer used SPI
* Rename some TraceUtil methods to ensure they are used properly
* Update TraceUtil to simplify initialization on the server-side using a
single boolean, that can also be enabled/disabled easily for the Shell
using the Shell's TraceCommand (enabled just means it will use the
GlobalOpenTelemetry instance, which is typically the autoconfigured
one, and disabled means it will use the NOOP explicitly, even if the
GlobalOpenTelemetry instance is set)
* Short-circuit span creation using an invalid span singleton if tracing
is disabled
* Improve getContext javadoc
---
.../accumulo/core/conf/ClientConfigGenerate.java | 2 +-
.../apache/accumulo/core/conf/ClientProperty.java | 6 -
.../org/apache/accumulo/core/conf/Property.java | 6 +-
.../accumulo/core/rpc/TraceProtocolFactory.java | 2 +-
.../core/spi/trace/OpenTelemetryFactory.java | 28 -----
.../org/apache/accumulo/core/trace/TraceUtil.java | 140 +++++++--------------
.../org/apache/accumulo/server/AbstractServer.java | 6 +-
.../accumulo/manager/tableOps/TraceRepo.java | 6 +-
shell/pom.xml | 4 +
.../main/java/org/apache/accumulo/shell/Shell.java | 2 -
.../accumulo/shell/commands/TraceCommand.java | 25 +++-
11 files changed, 74 insertions(+), 153 deletions(-)
diff --git
a/core/src/main/java/org/apache/accumulo/core/conf/ClientConfigGenerate.java
b/core/src/main/java/org/apache/accumulo/core/conf/ClientConfigGenerate.java
index a17dfe9..0ad0a8a 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/ClientConfigGenerate.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/ClientConfigGenerate.java
@@ -52,7 +52,7 @@ public class ClientConfigGenerate {
generateSection("Scanner", "scanner.");
generateSection("SSL", "ssl.");
generateSection("SASL", "sasl.");
- generateSection("Tracing", "general.opentelemetry.");
+ generateSection("Tracing", "trace.");
doc.close();
}
diff --git
a/core/src/main/java/org/apache/accumulo/core/conf/ClientProperty.java
b/core/src/main/java/org/apache/accumulo/core/conf/ClientProperty.java
index 38d8c30..a1d8431 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/ClientProperty.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/ClientProperty.java
@@ -123,12 +123,6 @@ public enum ClientProperty {
"Kerberos principal/primary that Accumulo servers use to login"),
// Trace
- @Experimental
- GENERAL_OPENTELEMETRY_ENABLED("general.opentelemetry.enabled", "false",
PropertyType.BOOLEAN,
- "Enables tracing functionality using OpenTelemetry.", "2.1.0", false),
- @Experimental
- GENERAL_OPENTELEMETRY_FACTORY("general.opentelemetry.factory", "",
PropertyType.CLASSNAME,
- "Name of class that implements OpenTelemetryFactory", "2.1.0", false),
@Deprecated(since = "2.1.0", forRemoval = true)
TRACE_SPAN_RECEIVERS("trace.span.receivers",
"org.apache.accumulo.tracer.ZooTraceClient",
"A list of span receiver classes to send trace spans"),
diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
index 4b8323a..ab8a21b 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
@@ -241,10 +241,8 @@ public enum Property {
"The maximum size of a message that can be sent to a server.", "1.5.0"),
@Experimental
GENERAL_OPENTELEMETRY_ENABLED("general.opentelemetry.enabled", "false",
PropertyType.BOOLEAN,
- "Enables tracing functionality using OpenTelemetry.", "2.1.0"),
- @Experimental
- GENERAL_OPENTELEMETRY_FACTORY("general.opentelemetry.factory", "",
PropertyType.CLASSNAME,
- "Name of class that implements OpenTelemetryFactory", "2.1.0"),
+ "Enables tracing functionality using OpenTelemetry (assuming
OpenTelemetry is configured).",
+ "2.1.0"),
GENERAL_SIMPLETIMER_THREADPOOL_SIZE("general.server.simpletimer.threadpool.size",
"1",
PropertyType.COUNT, "The number of threads to use for " +
"server-internal scheduled tasks",
"1.7.0"),
diff --git
a/core/src/main/java/org/apache/accumulo/core/rpc/TraceProtocolFactory.java
b/core/src/main/java/org/apache/accumulo/core/rpc/TraceProtocolFactory.java
index 5032afa..617811b 100644
--- a/core/src/main/java/org/apache/accumulo/core/rpc/TraceProtocolFactory.java
+++ b/core/src/main/java/org/apache/accumulo/core/rpc/TraceProtocolFactory.java
@@ -43,7 +43,7 @@ public class TraceProtocolFactory extends
TCompactProtocol.Factory {
@Override
public void writeMessageBegin(TMessage message) throws TException {
- span = TraceUtil.startClientSpan(this.getClass(), message.name);
+ span = TraceUtil.startClientRpcSpan(this.getClass(), message.name);
scope = span.makeCurrent();
super.writeMessageBegin(message);
}
diff --git
a/core/src/main/java/org/apache/accumulo/core/spi/trace/OpenTelemetryFactory.java
b/core/src/main/java/org/apache/accumulo/core/spi/trace/OpenTelemetryFactory.java
deleted file mode 100644
index 9c74365..0000000
---
a/core/src/main/java/org/apache/accumulo/core/spi/trace/OpenTelemetryFactory.java
+++ /dev/null
@@ -1,28 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.apache.accumulo.core.spi.trace;
-
-import java.util.function.Supplier;
-
-import io.opentelemetry.api.OpenTelemetry;
-
-/**
- * Configures and returns an instance of OpenTelemetry
- */
-public interface OpenTelemetryFactory extends Supplier<OpenTelemetry> {}
diff --git a/core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
b/core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
index 633e375..4dc0890 100644
--- a/core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
+++ b/core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
@@ -18,20 +18,14 @@
*/
package org.apache.accumulo.core.trace;
-import static org.apache.accumulo.core.Constants.APPNAME;
-import static org.apache.accumulo.core.Constants.VERSION;
-
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
import java.util.Map;
-import java.util.Properties;
-import org.apache.accumulo.core.classloader.ClassLoaderUtil;
+import org.apache.accumulo.core.Constants;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.conf.ClientProperty;
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.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -56,97 +50,41 @@ public class TraceUtil {
private static final String SPAN_FORMAT = "%s::%s";
- private static Tracer instance = null;
- private static boolean tracing = false;
+ private static volatile boolean enabled = true;
- private static void initializeInternals(OpenTelemetry ot) {
- instance = ot.getTracer(APPNAME, VERSION);
- tracing = !ot.equals(OpenTelemetry.noop());
- LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance:
{}", tracing,
- ot.getClass(), instance.getClass());
+ public static void initializeTracer(AccumuloConfiguration conf) {
+ enabled = conf.getBoolean(Property.GENERAL_OPENTELEMETRY_ENABLED);
+ logTracingState(false);
}
- /**
- * Use the property values in the client properties to call
- * {@link #initializeTracer(boolean, String)}
- *
- * @param clientProperties
- * the client Properties
- * @throws ReflectiveOperationException
- * unable to find or load class
- */
- public static void initializeTracer(Properties clientProperties)
- throws ReflectiveOperationException {
-
initializeTracer(ClientProperty.GENERAL_OPENTELEMETRY_ENABLED.getBoolean(clientProperties),
-
ClientProperty.GENERAL_OPENTELEMETRY_FACTORY.getValue(clientProperties));
+ public static void enable() {
+ enabled = true;
+ logTracingState(true);
}
- /**
- * Use the property values in the AccumuloConfiguration to call
- * {@link #initializeTracer(boolean, String)}
- *
- * @param conf
- * AccumuloConfiguration
- * @throws ReflectiveOperationException
- * unable to find or load class
- */
- public static void initializeTracer(AccumuloConfiguration conf)
- throws ReflectiveOperationException {
- initializeTracer(conf.getBoolean(Property.GENERAL_OPENTELEMETRY_ENABLED),
- conf.get(Property.GENERAL_OPENTELEMETRY_FACTORY));
+ public static void disable() {
+ enabled = false;
+ logTracingState(true);
}
- /**
- * 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
- * @throws ReflectiveOperationException
- * unable to find or load class
- */
- private static void initializeTracer(boolean enabled, String factoryClass)
- throws ReflectiveOperationException {
- if (instance == null) {
- OpenTelemetry ot = null;
- if (!enabled) {
- ot = OpenTelemetry.noop();
- } else if (factoryClass != null && !factoryClass.isEmpty()) {
- var clazz = ClassLoaderUtil.loadClass(factoryClass,
OpenTelemetryFactory.class);
- OpenTelemetryFactory factory =
clazz.getDeclaredConstructor().newInstance();
- ot = factory.get();
- LOG.info("OpenTelemetry configured and set from {}", clazz);
- } else {
- ot = GlobalOpenTelemetry.get();
- }
- initializeInternals(ot);
+ private static void logTracingState(boolean debug) {
+ var msg = "Trace enabled in Accumulo: {}, OpenTelemetry instance: {},
Tracer instance: {}";
+ var enabledInAccumulo = enabled ? "yes" : "no";
+ var openTelemetry = getOpenTelemetry();
+ var tracer = getTracer(openTelemetry);
+ if (debug) {
+ LOG.debug(msg, enabledInAccumulo, openTelemetry.getClass(),
tracer.getClass());
} else {
- LOG.warn("Tracer already initialized.");
+ LOG.info(msg, enabledInAccumulo, openTelemetry.getClass(),
tracer.getClass());
}
}
- /**
- * @return the Tracer set on the GlobalOpenTelemetry object
- */
- private static Tracer getTracer() {
- if (instance == null) {
- LOG.warn("initializeTracer not called, using
GlobalOpenTelemetry.getTracer()");
- instance = GlobalOpenTelemetry.getTracer(APPNAME, VERSION);
- tracing = !instance.equals(OpenTelemetry.noop().getTracer(APPNAME,
VERSION));
- LOG.info("Trace enabled: {}, Tracer is: {}", tracing,
instance.getClass());
- }
- return instance;
+ private static OpenTelemetry getOpenTelemetry() {
+ return enabled ? GlobalOpenTelemetry.get() : OpenTelemetry.noop();
}
- /**
- * @return true if an OpenTelemetry Tracer implementation has been set,
false if the NoOp Tracer
- * is being used.
- */
- public static boolean isTracing() {
- return tracing;
+ private static Tracer getTracer(OpenTelemetry ot) {
+ return ot.getTracer(Constants.APPNAME, Constants.VERSION);
}
public static Span startSpan(Class<?> caller, String spanName) {
@@ -157,26 +95,33 @@ public class TraceUtil {
return startSpan(caller, spanName, null, attributes, null);
}
- public static Span startClientSpan(Class<?> caller, String spanName) {
+ public static Span startClientRpcSpan(Class<?> caller, String spanName) {
return startSpan(caller, spanName, SpanKind.CLIENT, null, null);
}
- public static Span startServerSpan(Class<?> caller, String spanName, TInfo
tinfo) {
- return startSpan(caller, spanName, SpanKind.SERVER, null,
getContext(tinfo));
+ public static Span startFateSpan(Class<?> caller, String spanName, TInfo
tinfo) {
+ return startSpan(caller, spanName, null, null, tinfo);
+ }
+
+ public static Span startServerRpcSpan(Class<?> caller, String spanName,
TInfo tinfo) {
+ return startSpan(caller, spanName, SpanKind.SERVER, null, tinfo);
}
private static Span startSpan(Class<?> caller, String spanName, SpanKind
kind,
- Map<String,String> attributes, Context parent) {
+ Map<String,String> attributes, TInfo tinfo) {
+ if (!enabled) {
+ return Span.getInvalid();
+ }
final String name = String.format(SPAN_FORMAT, caller.getSimpleName(),
spanName);
- final SpanBuilder builder = getTracer().spanBuilder(name);
+ final SpanBuilder builder =
getTracer(getOpenTelemetry()).spanBuilder(name);
if (kind != null) {
builder.setSpanKind(kind);
}
if (attributes != null) {
attributes.forEach(builder::setAttribute);
}
- if (parent != null) {
- builder.setParent(parent);
+ if (tinfo != null) {
+ builder.setParent(getContext(tinfo));
}
return builder.startSpan();
}
@@ -192,7 +137,7 @@ public class TraceUtil {
* whether the exception is subsequently re-thrown
*/
public static void setException(Span span, Throwable e, boolean rethrown) {
- if (tracing) {
+ if (enabled) {
span.setStatus(StatusCode.ERROR);
span.recordException(e,
Attributes.builder().put(SemanticAttributes.EXCEPTION_TYPE,
e.getClass().getName())
@@ -217,13 +162,12 @@ public class TraceUtil {
* then be used in this process to continue the tracing. The Context is used
like:
*
* <pre>
- * Context remoteCtx = TracerFactory.getContext(tinfo);
- * Span span =
TracerFactory.getTracer().spanBuilder(name).setParent(remoteCtx).startSpan()
+ * Context remoteCtx = getContext(tinfo);
+ * Span span = tracer.spanBuilder(name).setParent(remoteCtx).startSpan()
* </pre>
*
* @param tinfo
- * tracing information
- * @return Context
+ * tracing information serialized over Thrift
*/
private static Context getContext(TInfo tinfo) {
return W3CTraceContextPropagator.getInstance().extract(Context.current(),
tinfo,
@@ -255,7 +199,7 @@ public class TraceUtil {
throw e.getCause();
}
}
- Span span = startServerSpan(instance.getClass(), method.getName(),
(TInfo) args[0]);
+ Span span = startServerRpcSpan(instance.getClass(), method.getName(),
(TInfo) args[0]);
try (Scope scope = span.makeCurrent()) {
return method.invoke(instance, args);
} catch (Exception e) {
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java
b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java
index 8dc280f..36dcf3a 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java
@@ -51,11 +51,7 @@ public abstract class AbstractServer implements
AutoCloseable, Runnable {
log.info("Instance " + context.getInstanceID());
context.init(appName);
ClassLoaderUtil.initContextFactory(context.getConfiguration());
- try {
- TraceUtil.initializeTracer(context.getConfiguration());
- } catch (ReflectiveOperationException e) {
- log.error("Error initializing tracing", e);
- }
+ TraceUtil.initializeTracer(context.getConfiguration());
if (context.getSaslParams() != null) {
// Server-side "client" check to make sure we're logged in as a user we
expect to be
context.enforceKerberosLogin();
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/TraceRepo.java
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/TraceRepo.java
index f2d2a78..d3b4a00 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/TraceRepo.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/TraceRepo.java
@@ -42,7 +42,7 @@ public class TraceRepo<T> implements Repo<T> {
@Override
public long isReady(long tid, T environment) throws Exception {
- Span span = TraceUtil.startServerSpan(repo.getClass(),
repo.getDescription(), tinfo);
+ Span span = TraceUtil.startFateSpan(repo.getClass(),
repo.getDescription(), tinfo);
try (Scope scope = span.makeCurrent()) {
return repo.isReady(tid, environment);
} catch (Exception e) {
@@ -55,7 +55,7 @@ public class TraceRepo<T> implements Repo<T> {
@Override
public Repo<T> call(long tid, T environment) throws Exception {
- Span span = TraceUtil.startServerSpan(repo.getClass(),
repo.getDescription(), tinfo);
+ Span span = TraceUtil.startFateSpan(repo.getClass(),
repo.getDescription(), tinfo);
try (Scope scope = span.makeCurrent()) {
Repo<T> result = repo.call(tid, environment);
if (result == null)
@@ -71,7 +71,7 @@ public class TraceRepo<T> implements Repo<T> {
@Override
public void undo(long tid, T environment) throws Exception {
- Span span = TraceUtil.startServerSpan(repo.getClass(),
repo.getDescription(), tinfo);
+ Span span = TraceUtil.startFateSpan(repo.getClass(),
repo.getDescription(), tinfo);
try (Scope scope = span.makeCurrent()) {
repo.undo(tid, environment);
} catch (Exception e) {
diff --git a/shell/pom.xml b/shell/pom.xml
index f5a28e9..107e31e 100644
--- a/shell/pom.xml
+++ b/shell/pom.xml
@@ -60,6 +60,10 @@
<artifactId>opentelemetry-api</artifactId>
</dependency>
<dependency>
+ <groupId>io.opentelemetry</groupId>
+ <artifactId>opentelemetry-context</artifactId>
+ </dependency>
+ <dependency>
<groupId>org.apache.accumulo</groupId>
<artifactId>accumulo-core</artifactId>
</dependency>
diff --git a/shell/src/main/java/org/apache/accumulo/shell/Shell.java
b/shell/src/main/java/org/apache/accumulo/shell/Shell.java
index d94d89e..8c595be 100644
--- a/shell/src/main/java/org/apache/accumulo/shell/Shell.java
+++ b/shell/src/main/java/org/apache/accumulo/shell/Shell.java
@@ -68,7 +68,6 @@ import org.apache.accumulo.core.data.Key;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.dataImpl.thrift.TConstraintViolationSummary;
import
org.apache.accumulo.core.tabletserver.thrift.ConstraintViolationException;
-import org.apache.accumulo.core.trace.TraceUtil;
import org.apache.accumulo.core.util.BadArgumentException;
import org.apache.accumulo.core.util.format.DefaultFormatter;
import org.apache.accumulo.core.util.format.Formatter;
@@ -353,7 +352,6 @@ public class Shell extends ShellOptions implements
KeywordExecutable {
}
}
try {
- TraceUtil.initializeTracer(clientProperties);
this.setTableName("");
accumuloClient =
Accumulo.newClient().from(clientProperties).as(principal, token).build();
context = (ClientContext) accumuloClient;
diff --git
a/shell/src/main/java/org/apache/accumulo/shell/commands/TraceCommand.java
b/shell/src/main/java/org/apache/accumulo/shell/commands/TraceCommand.java
index 988c0be..f7bc825 100644
--- a/shell/src/main/java/org/apache/accumulo/shell/commands/TraceCommand.java
+++ b/shell/src/main/java/org/apache/accumulo/shell/commands/TraceCommand.java
@@ -22,28 +22,43 @@ import java.io.IOException;
import org.apache.accumulo.core.trace.TraceUtil;
import org.apache.accumulo.core.util.BadArgumentException;
+import org.apache.accumulo.core.util.Pair;
import org.apache.accumulo.shell.Shell;
import org.apache.accumulo.shell.Shell.Command;
import org.apache.commons.cli.CommandLine;
import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.context.Scope;
public class TraceCommand extends Command {
- private Span span = null;
+ private Pair<Span,Scope> span;
+
+ public TraceCommand() {
+ // off by default
+ TraceUtil.disable();
+ }
@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")) {
+ TraceUtil.enable();
if (span == null) {
- span = TraceUtil.startSpan(Shell.class,
shellState.getAccumuloClient().whoami());
+ var spanOT = TraceUtil.startSpan(Shell.class,
shellState.getAccumuloClient().whoami());
+ var scopeOT = spanOT.makeCurrent();
+ span = new Pair<>(spanOT, scopeOT);
}
} else if (cl.getArgs()[0].equalsIgnoreCase("off")) {
if (span != null) {
- span.end();
- span = null;
+ try {
+ TraceUtil.disable();
+ span.getSecond().close();
+ } finally {
+ span.getFirst().end();
+ span = null;
+ }
} else {
shellState.getWriter().println("Not tracing");
}
@@ -52,7 +67,7 @@ public class TraceCommand extends Command {
fullCommand.indexOf(cl.getArgs()[0]));
}
} else if (cl.getArgs().length == 0) {
- shellState.getWriter().println(TraceUtil.isTracing() ? "on" : "off");
+ shellState.getWriter().println(span == null ? "off" : "on");
} else {
shellState.printException(new IllegalArgumentException(
"Expected 0 or 1 argument. There were " + cl.getArgs().length +
"."));
