Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add autoconfiguration wrapper artifact #2401

Merged
merged 22 commits into from
Jan 7, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.spi.OpenTelemetryFactory;
import io.opentelemetry.spi.trace.TracerProviderFactory;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nullable;

/**
Expand All @@ -30,7 +34,11 @@
* @see ContextPropagators
*/
public final class GlobalOpenTelemetry {
// Visible for testing
static final Logger logger = Logger.getLogger(GlobalOpenTelemetry.class.getName());

private static final Object mutex = new Object();

@Nullable private static volatile OpenTelemetry globalOpenTelemetry;

private GlobalOpenTelemetry() {}
Expand All @@ -48,6 +56,13 @@ public static OpenTelemetry get() {
if (globalOpenTelemetry == null) {
synchronized (mutex) {
if (globalOpenTelemetry == null) {

OpenTelemetry autoConfigured = maybeAutoConfigure();
if (autoConfigured != null) {
set(autoConfigured);
return autoConfigured;
}

OpenTelemetryFactory openTelemetryFactory = Utils.loadSpi(OpenTelemetryFactory.class);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike the first commit with PoC, I haven't removed the API SPI yet, that we would do in a separate PR.

if (openTelemetryFactory != null) {
set(openTelemetryFactory.create());
Expand Down Expand Up @@ -115,4 +130,31 @@ public static Tracer getTracer(String instrumentationName, String instrumentatio
public static ContextPropagators getPropagators() {
return get().getPropagators();
}

@Nullable
private static OpenTelemetry maybeAutoConfigure() {
final Class<?> openTelemetrySdkAutoConfiguration;
try {
openTelemetrySdkAutoConfiguration =
Class.forName("io.opentelemetry.sdk.autoconfigure.OpenTelemetrySdkAutoConfiguration");
} catch (ClassNotFoundException e) {
return null;
}

try {
Method initialize = openTelemetrySdkAutoConfiguration.getMethod("initialize");
return (OpenTelemetry) initialize.invoke(null);
} catch (NoSuchMethodException | IllegalAccessException e) {
throw new IllegalStateException(
"OpenTelemetrySdkAutoConfiguration detected on classpath "
+ "but could not invoke initialize method. This is a bug in OpenTelemetry.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could also be due to a restrictive security manager

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a public method, as far as I know there wouldn't be any situation that could be blocked by security manager, is there one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought security managers could block all reflection, but I wouldn't swear to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a security manager can block all reflection, even for public methods, and even just calls to "getMethod", although I would suspect that's extremely rare these days.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had no idea - do you think I should change the message? Wondering how realistic it is to disable public reflection vs adding noise to this code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just "This could be a bug in OpenTelemetry" ? Really, it doesn't matter all that much.

e);
} catch (InvocationTargetException t) {
logger.log(
Level.SEVERE,
"Error automatically configuring OpenTelemetry SDK. OpenTelemetry will not be enabled.",
t.getTargetException());
return null;
}
}
}
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ subprojects {
jsr305 : "com.google.code.findbugs:jsr305:${findBugsJsr305Version}",
prometheus_client : "io.prometheus:simpleclient:${prometheusVersion}",
prometheus_client_common : "io.prometheus:simpleclient_common:${prometheusVersion}",
prometheus_client_httpserver: "io.prometheus:simpleclient_httpserver:${prometheusVersion}",
jkwatson marked this conversation as resolved.
Show resolved Hide resolved
protobuf : "com.google.protobuf:protobuf-java",
protobuf_util : "com.google.protobuf:protobuf-java-util",
zipkin_reporter : "io.zipkin.reporter2:zipkin-reporter",
Expand Down
74 changes: 74 additions & 0 deletions sdk-extensions/autoconfigure/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
plugins {
id "java-library"
id "maven-publish"

id "org.unbroken-dome.test-sets"
id "ru.vyarus.animalsniffer"
}

description = 'OpenTelemetry SDK Auto-configuration'
ext.moduleName = "io.opentelemetry.sdk.autoconfigure"

testSets {
testConfigError
testFullConfig
testPrometheus
}

dependencies {
api project(':sdk:all'),
project(':sdk:metrics')

compileOnly project(':extensions:trace-propagators')
compileOnly project(':exporters:jaeger')
compileOnly project(':exporters:logging')
compileOnly project(':exporters:otlp:all')
compileOnly project(':exporters:otlp:metrics')
compileOnly project(':exporters:prometheus')
compileOnly libraries.prometheus_client_httpserver
compileOnly project(':exporters:zipkin')

testImplementation project(':proto'),
project(':sdk:testing'),
'com.linecorp.armeria:armeria-junit5',
'com.linecorp.armeria:armeria-grpc'
testRuntimeOnly 'io.grpc:grpc-netty-shaded'

testFullConfigImplementation project(':extensions:trace-propagators')
testFullConfigImplementation project(':exporters:jaeger')
testFullConfigImplementation project(':exporters:logging')
testFullConfigImplementation project(':exporters:otlp:all')
testFullConfigImplementation project(':exporters:otlp:metrics')
testFullConfigImplementation project(':exporters:prometheus')
testFullConfigImplementation libraries.prometheus_client_httpserver
testFullConfigImplementation project(':exporters:zipkin')

testConfigErrorImplementation project(':extensions:trace-propagators')
testConfigErrorImplementation project(':exporters:jaeger')
testConfigErrorImplementation project(':exporters:logging')
testConfigErrorImplementation project(':exporters:otlp:all')
testConfigErrorImplementation project(':exporters:otlp:metrics')
testConfigErrorImplementation project(':exporters:prometheus')
testConfigErrorImplementation libraries.prometheus_client_httpserver
testConfigErrorImplementation project(':exporters:zipkin')
testConfigErrorImplementation libraries.junit_pioneer

testPrometheusImplementation project(':exporters:prometheus')
testPrometheusImplementation libraries.prometheus_client_httpserver
}

testFullConfig {
environment("OTEL_RESOURCE_ATTRIBUTES", "service.name=test,cat=meow")
environment("OTEL_EXPORTER", "otlp,jaeger,zipkin")
environment("OTEL_PROPAGATORS", "tracecontext,baggage,b3,b3multi,jaeger,ottracer,xray")
environment("OTEL_BSP_SCHEDULE_DELAY_MILLIS", "10")
environment("OTEL_IMR_EXPORT_INTERVAL", "10")
environment("OTEL_EXPORTER_OTLP_HEADERS", "cat=meow,dog=bark")
environment("OTEL_EXPORTER_OTLP_TIMEOUT", "5000")
environment("OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT", "2")
}

testPrometheus {
environment("OTEL_EXPORTER", "prometheus")
environment("OTEL_IMR_EXPORT_INTERVAL", "10")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.autoconfigure;

final class ClasspathUtil {

@SuppressWarnings("UnusedException")
static void checkClassExists(String className, String featureName, String requiredLibrary) {
try {
Class.forName(className);
} catch (ClassNotFoundException unused) {
throw new ConfigurationException(
featureName
+ " enabled but "
+ requiredLibrary
+ " not found on classpath. "
+ "Make sure to add it as a dependency to enable this feature.");
}
}

private ClasspathUtil() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.autoconfigure;

import java.util.AbstractMap;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.stream.Collectors;
import javax.annotation.Nullable;

class ConfigProperties {

static ConfigProperties get() {
return new ConfigProperties(System.getProperties(), System.getenv());
}

// Visible for testing
@SuppressWarnings({"unchecked", "rawtypes"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this supression is needed any more.

anuraaga marked this conversation as resolved.
Show resolved Hide resolved
static ConfigProperties createForTest(Map<String, String> properties) {
return new ConfigProperties((Map) properties, Collections.emptyMap());
}

private final Map<String, String> config;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be up above the static methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh - I'm going to have trouble getting used to fields being so far from the constructor :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's why static methods should go at the bottom of the class 😁


private ConfigProperties(
Map<Object, Object> systemProperties, Map<String, String> environmentVariables) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the left map could be Map<?,?> then you don't have to cast above.

Map<String, String> config = new HashMap<>();
environmentVariables.forEach(
(name, value) -> config.put(name.toLowerCase(Locale.ROOT).replace('_', '.'), value));
systemProperties.forEach(
(key, value) -> config.put(((String) key).toLowerCase(Locale.ROOT), (String) value));

this.config = config;
}

@Nullable
String getString(String name) {
return config.get(name);
}

@Nullable
@SuppressWarnings("UnusedException")
Integer getInt(String name) {
String value = config.get(name);
if (value == null || value.isEmpty()) {
return null;
}
try {
return Integer.parseInt(value);
} catch (NumberFormatException ex) {
throw newInvalidPropertyException(name, value, "integer");
}
}

@Nullable
@SuppressWarnings("UnusedException")
Long getLong(String name) {
String value = config.get(name);
if (value == null || value.isEmpty()) {
return null;
}
try {
return Long.parseLong(value);
} catch (NumberFormatException ex) {
throw newInvalidPropertyException(name, value, "long");
}
}

@Nullable
@SuppressWarnings("UnusedException")
Double getDouble(String name) {
String value = config.get(name);
if (value == null || value.isEmpty()) {
return null;
}
try {
return Double.parseDouble(value);
} catch (NumberFormatException ex) {
throw newInvalidPropertyException(name, value, "double");
}
}

List<String> getCommaSeparatedValues(String name) {
String value = config.get(name);
if (value == null) {
return Collections.emptyList();
}
return Arrays.stream(value.split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.toList());
}

Map<String, String> getCommaSeparatedMap(String name) {
return getCommaSeparatedValues(name).stream()
.map(
keyValuePair ->
Arrays.stream(keyValuePair.split("=", 2))
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.toList()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 4 lines of code could be extracted to a method like "filterBlanksAndNulls" and re-used above at line 96-99

.map(
splitKeyValuePairs -> {
if (splitKeyValuePairs.size() != 2) {
throw new ConfigurationException(
"Map property key missing value: " + name + "=" + config.get(name));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit. if you have an entry like =f it's technically missing the key, not the value. ;)

}
return new AbstractMap.SimpleImmutableEntry<>(
splitKeyValuePairs.get(0), splitKeyValuePairs.get(1));
})
// If duplicate keys, prioritize later ones similar to duplicate system properties on a
// Java command line.
.collect(
Collectors.toMap(
Map.Entry::getKey, Map.Entry::getValue, (first, next) -> next, LinkedHashMap::new));
}

boolean getBoolean(String name) {
return Boolean.parseBoolean(config.get(name));
}

private static ConfigurationException newInvalidPropertyException(
String name, String value, String type) {
throw new ConfigurationException(
"Invalid value for property " + name + "=" + value + ". Must be a " + type + ".");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.sdk.autoconfigure;

/** An exception that is thrown if the user-provided configuration is invalid. */
public final class ConfigurationException extends RuntimeException {

private static final long serialVersionUID = 4717640118051490483L;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary or useful. No one should be using default serialization for anything, anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Java throws a warning which fails our build if I don't have it (agree no one should serialize it :)

warning: [serial] serializable class ConfigurationException has no definition of serialVersionUID

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that must be an errorprone warning. I can't imagine that javac would emit that.


ConfigurationException(String message) {
super(message);
}
}
Loading