You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/11/18 18:16:43 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request #2363: Minor improvements to opentelemetry tooling
ctubbsii opened a new pull request #2363:
URL: https://github.com/apache/accumulo/pull/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
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2363: Minor improvements to opentelemetry tooling
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2363:
URL: https://github.com/apache/accumulo/pull/2363#discussion_r754550047
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -56,97 +52,32 @@
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);
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance: {}", enabled,
Review comment:
Consolidated log message handling in 8128c11a7a841756e056e229ef01cf26f9b85f86
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2363: Minor improvements to opentelemetry tooling
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2363:
URL: https://github.com/apache/accumulo/pull/2363#discussion_r754541157
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -56,97 +52,32 @@
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);
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance: {}", enabled,
Review comment:
I can try to consolidate the log messages. I didn't before because one was at INFO and the others at DEBUG, and it didn't seem to save much.
I don't think it makes sense to do the additional check in `getOpenTelemetry()`, since that won't have any effect on its behavior. I created that separate method instead of inlining it into `getTracer()` specifically for the purposes of being able to log it. However, I think I do need to improve that slightly, so we don't log one OpenTelemetry, and then it changes, and we log a different OpenTelemetry's tracer. I can make sure `getTracer` always uses the OpenTelemetry object retrieved from `getOpenTelemetry()`.
As for it still logging that trace is enabled, I think it's important that we log the setting that Accumulo has been configured with. If it's still using the NOOP implementation, that will be revealed in the remainder of the log message when we print the specific name of the class. I can update the format string to be more clear about what "true" means, but I'd rather not modify it to mean that we've set it to enabled *and* that a non-NOOP implementation is configured. I'd rather it mean just that we've set it to enabled, and leave the rest of the log message to report on the implementation being used.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2363: Minor improvements to opentelemetry tooling
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2363:
URL: https://github.com/apache/accumulo/pull/2363#discussion_r753200841
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -56,97 +52,32 @@
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);
+ LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance: {}", enabled,
Review comment:
In testing I set `general.opentelemetry.enabled` to `true`, but did not have the java agent jar or any other OpenTelemetry jar on the classpath. It still logged that trace was enabled. I think in this method and in `enable()` we need to also test whether the result of `getOpenTelemetry()` is equal to `OpenTelemetry.noop()`. We should probably consolidate the logging in `initializeTracer`, `enable`, and `disable` to one method. Something like:
```
private static void logMsg() {
boolean isEnabled = enabled && (!getOpenTelemetry().equals(OpenTelemetry.noop());
LOG.info("Trace enabled: {}, OpenTelemetry instance: {}, Tracer instance: {}", isEnabled,
getOpenTelemetry().getClass(), getTracer().getClass());
}
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #2363: Minor improvements to opentelemetry tooling
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2363:
URL: https://github.com/apache/accumulo/pull/2363
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2363: Minor improvements to opentelemetry tooling
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2363:
URL: https://github.com/apache/accumulo/pull/2363#discussion_r752506402
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/ClientConfigGenerate.java
##########
@@ -52,7 +52,7 @@ void generate() {
generateSection("Scanner", "scanner.");
generateSection("SSL", "ssl.");
generateSection("SASL", "sasl.");
- generateSection("Tracing", "general.opentelemetry.");
+ generateSection("Tracing", "trace.");
Review comment:
This change is just a simple revert to what was there before. It will preserve the deprecated client trace properties in the docs.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2363: Minor improvements to opentelemetry tooling
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2363:
URL: https://github.com/apache/accumulo/pull/2363#discussion_r752512239
##########
File path: core/src/main/java/org/apache/accumulo/core/trace/TraceUtil.java
##########
@@ -56,97 +52,32 @@
private static final String SPAN_FORMAT = "%s::%s";
- private static Tracer instance = null;
- private static boolean tracing = false;
+ private static volatile boolean enabled = true;
Review comment:
Leaving this on by default means client applications will default to using GlobalOpenTelemetry.get(), and can configure OpenTelemetry (or not) at their discretion. This PR switches this to false by default for server processes that load from the server config file, because the default value of the config option is false. The shell also switches this to false by default, so users can rely on the `trace on` / `trace off` command to toggle it. For the shell, it would also still need to be configured via the class path / autoconfigure (or javaagent) and config properties like other clients.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org