-
Notifications
You must be signed in to change notification settings - Fork 124
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
Changes from 6 commits
3b34c35
a774e18
08d7e75
b9b2ca4
cff529a
33a41d8
40aa3d9
7696ec1
a02a58f
ae1d1c2
7e3ae3d
53ee029
233b2db
ae7ff1f
2e7400d
cad17b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,12 +8,14 @@ | |
import io.opentelemetry.api.OpenTelemetry; | ||
import io.opentelemetry.api.trace.Tracer; | ||
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; | ||
|
@@ -25,16 +27,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 { | ||
|
||
|
@@ -89,52 +82,39 @@ 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( | ||
() -> { | ||
Map<String, String> properties = new HashMap<>(); | ||
if ((OtelUtils.getSysPropOrEnvVar("otel.traces.exporter") == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Instead of The default value for exporters is none, unless endpoint is explicitly passed in which case it is OTLP. Former just seems simpler to me, and also fits better with non-otlp exporters which have their own endpoint variables. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think this comment is still applicable. @cyrille-leclerc what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This change would make I have few questions please:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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
The behavior shouldn't be different since the default value of OTEL_TRACES_EXPORTER is 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.
This is an interesting topic - it doesn't affect OTel "normally" since exporters all default to the value There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
autoConfiguredSdkBuilder.addResourceCustomizer( | ||
(resource, configProperties) -> | ||
Resource.builder() | ||
.putAll(resource) | ||
.put(ResourceAttributes.SERVICE_VERSION, runtimeInformation.getMavenVersion()) | ||
.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( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.maven.resources; | ||
|
||
import io.opentelemetry.maven.semconv.MavenOtelSemanticAttributes; | ||
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties; | ||
import io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider; | ||
import io.opentelemetry.sdk.resources.Resource; | ||
import io.opentelemetry.sdk.resources.ResourceBuilder; | ||
import io.opentelemetry.semconv.resource.attributes.ResourceAttributes; | ||
|
||
public class MavenResourceProvider implements ResourceProvider { | ||
@Override | ||
public Resource createResource(ConfigProperties config) { | ||
ResourceBuilder resourceBuilder = Resource.builder(); | ||
resourceBuilder.put( | ||
ResourceAttributes.SERVICE_NAME, | ||
MavenOtelSemanticAttributes.ServiceNameValues.SERVICE_NAME_VALUE); | ||
return resourceBuilder.build(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
io.opentelemetry.maven.resources.MavenResourceProvider |
This file was deleted.
There was a problem hiding this comment.
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 :)