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

[Maven-Extension] Use Auto Configure Otel SDK Builder #132

Merged
merged 16 commits into from
Nov 27, 2021
Merged
Show file tree
Hide file tree
Changes from 13 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,10 +10,13 @@
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.maven.semconv.MavenOtelSemanticAttributes;
import io.opentelemetry.sdk.OpenTelemetrySdk;
import io.opentelemetry.sdk.autoconfigure.OpenTelemetrySdkAutoConfiguration;
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdkBuilder;
import io.opentelemetry.sdk.common.CompletableResultCode;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.trace.export.SpanExporter;
import io.opentelemetry.semconv.resource.attributes.ResourceAttributes;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import org.apache.maven.rtinfo.RuntimeInformation;
Expand All @@ -25,16 +28,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Service to configure the {@link OpenTelemetry} instance.
*
* <p>Rely on the OpenTelemetry SDK AutoConfiguration extension. Parameters are passed as system
* properties.
*
* <p>TODO: verify how we could use a composite {@link
* io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties} combining the config passed by JVM
* system properties and environment variables with overrides injected by the Otel Maven Extension
*/
/** Service to configure the {@link OpenTelemetry} instance. */
@Component(role = OpenTelemetrySdkService.class, hint = "opentelemetry-service")
public final class OpenTelemetrySdkService implements Initializable, Disposable {

Expand Down Expand Up @@ -89,52 +83,43 @@ public synchronized void dispose() {
@Override
public void initialize() throws InitializationException {
logger.debug("OpenTelemetry: initialize OpenTelemetrySdkService...");
if (StringUtils.isBlank(
OtelUtils.getSystemPropertyOrEnvironmentVariable(
"otel.exporter.otlp.endpoint", "OTEL_EXPORTER_OTLP_ENDPOINT", null))) {
logger.debug(
"OpenTelemetry: No -Dotel.exporter.otlp.endpoint property or OTEL_EXPORTER_OTLP_ENDPOINT "
+ "environment variable found, use a NOOP OpenTelemetry SDK");
} else {
{
// Don't use a {@code io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider} to inject
// Maven runtime attributes due to a classloading issue when loading the Maven OpenTelemetry
// extension as a pom.xml {@code <extension>}.
String initialCommaSeparatedAttributes =
OtelUtils.getSystemPropertyOrEnvironmentVariable(
"otel.resource.attributes", "OTEL_RESOURCE_ATTRIBUTES", "");
Map<String, String> attributes =
OtelUtils.getCommaSeparatedMap(initialCommaSeparatedAttributes);

// service.name
String serviceName =
OtelUtils.getSystemPropertyOrEnvironmentVariable(
"otel.service.name", "OTEL_SERVICE_NAME", null);

if (!attributes.containsKey(ResourceAttributes.SERVICE_NAME.getKey())
&& StringUtils.isBlank(serviceName)) {
// service.name is not defined in passed configuration, we define it
attributes.put(
ResourceAttributes.SERVICE_NAME.getKey(),
MavenOtelSemanticAttributes.ServiceNameValues.SERVICE_NAME_VALUE);
}

// service.version
final String mavenVersion = this.runtimeInformation.getMavenVersion();
if (!attributes.containsKey(ResourceAttributes.SERVICE_VERSION.getKey())) {
attributes.put(ResourceAttributes.SERVICE_VERSION.getKey(), mavenVersion);
}

String newCommaSeparatedAttributes = OtelUtils.getCommaSeparatedString(attributes);
logger.debug(
"OpenTelemetry: Initial resource attributes: {}", initialCommaSeparatedAttributes);
logger.debug("OpenTelemetry: Use resource attributes: {}", newCommaSeparatedAttributes);
System.setProperty("otel.resource.attributes", newCommaSeparatedAttributes);
}

this.openTelemetrySdk = OpenTelemetrySdkAutoConfiguration.initialize(false);
this.openTelemetry = this.openTelemetrySdk;
}
AutoConfiguredOpenTelemetrySdkBuilder autoConfiguredSdkBuilder =
AutoConfiguredOpenTelemetrySdk.builder();

// SDK CONFIGURATION PROPERTIES
autoConfiguredSdkBuilder.addPropertiesSupplier(
Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be cool if we can follow with populating this with XML properties from the POM file :)

() -> {
Map<String, String> properties = new HashMap<>();
if ((OtelUtils.getSysPropOrEnvVar("otel.traces.exporter") == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned it in another comment but how about just default to none regardless of endpoint variable? I think it's reasonable enough to export to localhost, and going from zero to two properties for a remote endpoint isn't that much harder than going from zero to one.

It seems simpler to document, just
The default value for exporter is none
The default value for endpoint is localhost:4317

Instead of

The default value for exporters is none, unless endpoint is explicitly passed in which case it is OTLP.
And endpoint effectively doesn't have a default value.

Former just seems simpler to me, and also fits better with non-otlp exporters which have their own endpoint variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

💯 The change you propose would simplify integration of the Otel SDK in apps like Maven or Jenkins.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I didn't mean changing the default of the OTel SDK itself (which is defined by the OTel spec) but just here, removing this if statement. We have a general stance of avoiding having default values of two variables associated with each other

open-telemetry/opentelemetry-specification#1895 (comment)

Which I think we have here. I don't see the usability drop to be significant enough if a user needs to set both OTEL_TRACES_EXPORTER and OTEL_EXPORTER_OTLP_ENDPOINT to send to a remote backend to not follow this general policy. Is it ok?

Copy link
Member

Choose a reason for hiding this comment

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

I've been trying to figure out from the history but can't seem to.

Is this comment still applicable @anuraaga or was it resolved @cyrille-leclerc?

Happy to review/approve when it's clear

Copy link
Contributor

Choose a reason for hiding this comment

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

Think this comment is still applicable. @cyrille-leclerc what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the delay, I was not sure to understand.

Do you mean that we would change the default behaviour of the "OpenTelemetry SDK Autoconfigure" so that the default value for OTEL_TRACES_EXPORTER who no longer be otlp but be none (official doc here)?

This change would make OTEL_TRACES_EXPORTER behave the same way OTEL_METRICS_EXPORTER behave, correct?

I have few questions please:

  • Is it correct that on the Otel Java agent, OTEL_TRACES_EXPORTER defaults to otlp when OTEL_METRICS_EXPORTER defaults to none making many users missing Java metrics not catching this subtle nuance of default value?
  • Would this be a breaking change for Otel Java Agent with many existing deployments not defining any of OTEL_TRACES_EXPORTER and OTEL_EXPORTER_OTLP_ENDPOINT, these deployments would suddenly loose tracing
  • The duality OTEL_TRACES_EXPORTER / OTEL_METRICS_EXPORTER is probably becoming verbose boilerplate with the rise of Otel metrics, including the rising support of metrics in observability backends (e.g. Jaeger is adding support for metrics via PromQL). This verbosity will even increase with the introduction of OTEL_LOGS_EXPORTER soon. Would it make sense to introduce a generic OTEL_EXPORTER so that the Java SDK exports all the signals it collects to the specified exporter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I wasn't clear - I don't mean to change the behavior of SDK autoconfigure (specifically, code in the opentelemetry-java repo) but just to change the defaults here. I'd like to remove handling of endpoint here so that

  • No settings: No export
  • OTEL_EXPORTER_OTLP_ENDPOINT=opentelemetry.io: No export
  • OTEL_TRACES_EXPORTER=otlp: Exports to default localhost address
  • OTEL_TRACES_EXPORTER=otlp OTEL_EXPORTER_OTLP_ENDPOINT=opentelemetry.io: Exports to opentelemetry.io

I know you are trying to make it simpler to use a remote endpoint, but I believe this provides a more consistent experience with our normal conventions, and don't believe the user experience difference for users of a remote endpoint to be negative enough in practice.

Notably, one issue I find with the current behavior is that
OTEL_TRACES_EXPORTER has a default value of none. Yet these two sets have different behavior

  • OTEL_EXPORTER_OTLP_ENDPONT=opentelemetry.io: Exports
  • OTEL_TRACES_EXPORTER=none OTEL_EXPORTER_OTLP_ENDPOINT=opentelemetry.io: Doesn't export

The behavior shouldn't be different since the default value of OTEL_TRACES_EXPORTER is none - we've just added a confusing caveat that the default is different based on the presence of another variable, something we would like to avoid.

This seems like an inconsistent experience. Does it make sense? I can add a commit directly to the PR if things are still unclear, let me know.

Would it make sense to introduce a generic OTEL_EXPORTER so that the Java SDK exports all the signals it collects to the specified exporter?

This is an interesting topic - it doesn't affect OTel "normally" since exporters all default to the value otlp. There is no value other than otlp that would satisfy OTEL_EXPORTER because zipkin doesn't do metrics, etc. It does affect this sort of use case where it is a good idea to change the normal default of otlp to none - we would need to discuss in spec if that means the OTEL_EXPORTER variable would be good to have for this use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for you patience @anuraaga , I hope I understood well your point, please check the updated code, it's greatly simplified.

&& (OtelUtils.getSysPropOrEnvVar("otel.exporter.otlp.endpoint") == null)
&& (OtelUtils.getSysPropOrEnvVar("otel.exporter.otlp.traces.endpoint") == null)) {
// Change default of "otel.traces.exporter" from "otlp" to "none" unless
// "otel.exporter.otlp.endpoint" or "otel.exporter.otlp.traces.endpoint" is defined
properties.put("otel.traces.exporter", "none");
}
return properties;
});

// SDK RESOURCE
// don't use the `io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider` framework because we
// need to get the `RuntimeInformation` component injected by Plexus
autoConfiguredSdkBuilder.addResourceCustomizer(
(resource, configProperties) ->
Resource.builder()
.putAll(resource)
.put(ResourceAttributes.SERVICE_VERSION, runtimeInformation.getMavenVersion())
.put(
ResourceAttributes.SERVICE_NAME, MavenOtelSemanticAttributes.SERVICE_NAME_VALUE)
.build());

// BUILD SDK
AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk =
autoConfiguredSdkBuilder.build();
logger.debug(
"OpenTelemetry: OpenTelemetry SDK initialized with "
+ OtelUtils.prettyPrintSdkConfiguration(autoConfiguredOpenTelemetrySdk));
this.openTelemetrySdk = autoConfiguredOpenTelemetrySdk.getOpenTelemetrySdk();
this.openTelemetry = this.openTelemetrySdk;

String mojosInstrumentationEnabledAsString =
System.getProperty(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,64 +5,58 @@

package io.opentelemetry.maven;

import java.util.AbstractMap;
import java.util.Arrays;
import java.util.HashMap;
import com.google.common.collect.Lists;
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import io.opentelemetry.sdk.resources.Resource;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.stream.Collectors;
import javax.annotation.CheckForNull;
import javax.annotation.Nullable;

final class OtelUtils {
protected static String prettyPrintSdkConfiguration(
AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk) {
List<String> configAttributeNames =
Lists.newArrayList(
cyrille-leclerc marked this conversation as resolved.
Show resolved Hide resolved
"otel.traces.exporter",
"otel.metrics.exporter",
"otel.exporter.otlp.endpoint",
"otel.exporter.otlp.traces.endpoint",
"otel.exporter.otlp.metrics.endpoint",
"otel.exporter.jaeger.endpoint",
"otel.exporter.prometheus.port",
"otel.resource.attributes",
"otel.service.name");

public static String getCommaSeparatedString(Map<String, String> keyValuePairs) {
return keyValuePairs.entrySet().stream()
.map(keyValuePair -> keyValuePair.getKey() + "=" + keyValuePair.getValue())
.collect(Collectors.joining(","));
}

public static Map<String, String> getCommaSeparatedMap(String commaSeparatedKeyValuePairs) {
if (StringUtils.isBlank(commaSeparatedKeyValuePairs)) {
return new HashMap<>();
ConfigProperties sdkConfig = autoConfiguredOpenTelemetrySdk.getConfig();
Map<String, String> configurationAttributes = new LinkedHashMap<>();
for (String attributeName : configAttributeNames) {
final String attributeValue = sdkConfig.getString(attributeName);
if (attributeValue != null) {
configurationAttributes.put(attributeName, attributeValue);
}
}
return filterBlanksAndNulls(commaSeparatedKeyValuePairs.split(",")).stream()
.map(keyValuePair -> filterBlanksAndNulls(keyValuePair.split("=", 2)))
.map(
splitKeyValuePairs -> {
if (splitKeyValuePairs.size() != 2) {
throw new RuntimeException(
"Invalid key-value pair: " + commaSeparatedKeyValuePairs);
}
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));
}

private static List<String> filterBlanksAndNulls(String[] values) {
return Arrays.stream(values)
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.toList());
Resource sdkResource = autoConfiguredOpenTelemetrySdk.getResource();

return "Configuration: "
+ configurationAttributes.entrySet().stream()
.map(entry -> entry.getKey() + "=" + entry.getValue())
.collect(Collectors.joining(", "))
+ ", Resource: "
+ sdkResource.getAttributes();
}

@CheckForNull
public static String getSystemPropertyOrEnvironmentVariable(
String systemPropertyName, String environmentVariableName, @Nullable String defaultValue) {
String systemProperty = System.getProperty(systemPropertyName);
if (StringUtils.isNotBlank(systemProperty)) {
return systemProperty;
}
String environmentVariable = System.getenv(environmentVariableName);
if (StringUtils.isNotBlank(environmentVariable)) {
return environmentVariable;
public static String getSysPropOrEnvVar(String systemPropertyName) {
String systemPropertyValue = System.getProperty(systemPropertyName);
if (systemPropertyValue != null) {
return systemPropertyValue;
}
return defaultValue;
String environmentVariableName = systemPropertyName.replace('.', '_').toUpperCase(Locale.ROOT);
return System.getenv().get(environmentVariableName);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,5 @@ public class MavenOtelSemanticAttributes {
public static final AttributeKey<String> MAVEN_EXECUTION_LIFECYCLE_PHASE =
stringKey("maven.execution.lifecyclePhase");

public static final class ServiceNameValues {
public static final String SERVICE_NAME_VALUE = "maven";

private ServiceNameValues() {}
}
public static final String SERVICE_NAME_VALUE = "maven";
}

This file was deleted.