Add autoconfiguration wrapper artifact #2401

anuraaga · 2020-12-23T08:40:00Z

This implements autoconfiguration moved into a separate layer as discussed a few times before. It's aim is to remove autoconfiguration from the core SDK so frameworks with their own configuration system, e.g., sleuth, can use it with full control, and other users can rely on our autoconfiguration if they want. Since we have control of the artifact, while slightly weird, it seems reasonable for us to automatically call into it with reflection, and otherwise we just return no-op (and log in the case of a misconfigured SDK as in #2396).

I mostly followed the parameters supported by the java agent except

Updated names to use latest from spec, some small tweaks
Didn't add support for "max attributes per event" type variables since not in spec
Didn't add jaeger_thrift Consider removing jaeger-thrift exporter from default agent opentelemetry-java-instrumentation#1973

anuraaga · 2020-12-23T08:41:23Z

...gure/src/main/java/io/opentelemetry/sdk/autoconfigure/OpenTelemetrySdkAutoConfiguration.java

+
+  private static void configureOtlpMetrics(
+      ConfigProperties config, SdkMeterProvider meterProvider) {
+    OtlpGrpcMetricExporterBuilder builder = OtlpGrpcMetricExporter.builder();


Will need to add linkage error guards to return better error messages for when a configuration option is set but implementation not available.

anuraaga · 2020-12-23T08:42:44Z

/cc @marcingrzejszczak you might be interested in this too

marcingrzejszczak · 2020-12-23T10:40:13Z

@anuraaga thanks for mentioning me! BTW can you start mentioning @jonatan-ivanov too (that's why I suggested @jkwatson to create a Github Team and just add people to the team and mention that team instead of concrete people).

build.gradle

jkwatson · 2020-12-23T17:16:06Z

@anuraaga thanks for mentioning me! BTW can you start mentioning @jonatan-ivanov too (that's why I suggested @jkwatson to create a Github Team and just add people to the team and mention that team instead of concrete people).

It turns out that I don't have permissions to create a team (or at least I get a 404 from github when trying to click the "new team" link). So, @bogdandrutu will have to do this, I think, unless he gives me (and @anuraaga) owner permissions on the repository.

jkwatson · 2020-12-23T17:27:18Z

...gure/src/main/java/io/opentelemetry/sdk/autoconfigure/OpenTelemetrySdkAutoConfiguration.java

+        .build();
+  }
+
+  private static void configureOtlpMetrics(


I think I'd break each of these methods out into their own class so it's clear how to add new ones to the auto-config, rather than having to dig into this beast to find them. :)

jkwatson · 2020-12-23T20:37:18Z

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

+    } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
+      throw new IllegalStateException(
+          "OpenTelemetrySdkAutoConfiguration detected on classpath "
+              + "but could not invoke initialize method. This is a bug in OpenTelemetry.",


this could also be due to a restrictive security manager

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?

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

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.

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.

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

jkwatson · 2020-12-23T20:42:34Z

this is pretty slick. It's SPI (albeit a slightly different flavor, with the builder), but it doesn't demand SPI be used. It makes it easy to manually auto-configure or manually configure, or mix-and-match however you like.

👍 to this approach. Obviously, if we want to go this route, we'll want to write some tests for the configuration (or move them over from the existing configuration code/tests), and do some refactor/cleanup so it's not one enormous class, but as a proof-of-concept, this is very nice. 🚀

… into autoconfigurre

codecov · 2020-12-25T05:13:49Z

Codecov Report

Merging #2401 (31853bc) into master (0dac3f6) will increase coverage by 87.85%.
The diff coverage is 41.44%.

@@              Coverage Diff              @@
##             master    #2401       +/-   ##
=============================================
+ Coverage          0   87.85%   +87.85%     
- Complexity        0     2692     +2692     
=============================================
  Files             0      310      +310     
  Lines             0     9017     +9017     
  Branches          0      964      +964     
=============================================
+ Hits              0     7922     +7922     
- Misses            0      768      +768     
- Partials          0      327      +327

Impacted Files	Coverage Δ	Complexity Δ
...sdk/autoconfigure/MetricExporterConfiguration.java	`2.04% <2.04%> (ø)`	`2.00 <2.00> (?)`
...try/sdk/autoconfigure/PropagatorConfiguration.java	`5.88% <5.88%> (ø)`	`1.00 <1.00> (?)`
...y/sdk/autoconfigure/SpanExporterConfiguration.java	`5.88% <5.88%> (ø)`	`2.00 <2.00> (?)`
...toconfigure/OpenTelemetrySdkAutoConfiguration.java	`10.71% <10.71%> (ø)`	`1.00 <1.00> (?)`
...java/io/opentelemetry/api/GlobalOpenTelemetry.java	`61.11% <27.77%> (ø)`	`11.00 <2.00> (?)`
...opentelemetry/sdk/autoconfigure/ClasspathUtil.java	`40.00% <40.00%> (ø)`	`1.00 <1.00> (?)`
...sdk/autoconfigure/TracerProviderConfiguration.java	`83.33% <83.33%> (ø)`	`20.00 <20.00> (?)`
...ntelemetry/sdk/autoconfigure/ConfigProperties.java	`86.00% <86.00%> (ø)`	`23.00 <23.00> (?)`
...etry/sdk/autoconfigure/ConfigurationException.java	`100.00% <100.00%> (ø)`	`1.00 <1.00> (?)`
...elemetry/sdk/testing/junit4/OpenTelemetryRule.java	`95.83% <0.00%> (ø)`	`7.00% <0.00%> (?%)`
... and 309 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dac3f6...06048d4. Read the comment docs.

… into autoconfigurre

anuraaga

I have a bit more grungework in adding unit tests for ConfigProperties and some failure cases, but I think this is mostly ready for another look

anuraaga · 2021-01-04T09:28:11Z

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

+            set(autoConfigured);
+            return autoConfigured;
+          }
+
          OpenTelemetryFactory openTelemetryFactory = Utils.loadSpi(OpenTelemetryFactory.class);


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

anuraaga · 2021-01-04T09:28:36Z

buildscripts/checkstyle.xml

@@ -55,20 +55,6 @@
             value="LITERAL_TRY, LITERAL_FINALLY, LITERAL_IF, LITERAL_ELSE, LITERAL_SWITCH"/>
        </module>
        <module name="NeedBraces"/>
-        <module name="LeftCurly"/>


anuraaga · 2021-01-04T09:29:45Z

...gure/src/main/java/io/opentelemetry/sdk/autoconfigure/OpenTelemetrySdkAutoConfiguration.java

+/**
+ * Auto-configuration for the OpenTelemetry SDK. As an alternative to programmatically configuring
+ * the SDK using {@link OpenTelemetrySdk#builder()}, this package can be used to automatically
+ * configure the SDK using environment properties specified by OpenTelemetry.


I'm hoping to link to a docs page from here rather than outlining all properties here - presuably autoconfiguration users are the last to read the javadocs

jkwatson · 2021-01-04T16:15:27Z

This is looking like a fantastic start. @anuraaga What do you think needs to be done to get it out of draft mode? I'd love to move forward with this ASAP, so we can move toward a 1.0 release.

anuraaga · 2021-01-04T23:50:29Z

@jkwatson Just a few more boring unit tests I'll knock out today :)

anuraaga · 2021-01-05T04:36:17Z

@jkwatson Ok should be good to go

jkwatson · 2021-01-05T19:26:59Z

...figure/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/SdkTracerProviderConfigurer.java

+/**
+ * A service provider interface (SPI) for performing additional programmatic configuration of a
+ * {@link SdkTracerProviderBuilder} during initialization. When using auto-configuration, you should
+ * prefer to use system properties for configuration, but this may be useful to register components


Suggested change

* prefer to use system properties for configuration, but this may be useful to register components

* prefer to use system properties or environment variables for configuration, but this may be useful to register components

jkwatson · 2021-01-05T19:31:43Z

...ensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ConfigProperties.java

+    return new ConfigProperties((Map) properties, Collections.emptyMap());
+  }
+
+  private final Map<String, String> config;


this should be up above the static methods

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

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

jkwatson · 2021-01-05T19:33:17Z

...ensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ConfigProperties.java

+  private final Map<String, String> config;
+
+  private ConfigProperties(
+      Map<Object, Object> systemProperties, Map<String, String> environmentVariables) {


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

jkwatson · 2021-01-05T20:25:43Z

...ensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ConfigProperties.java

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


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

jkwatson · 2021-01-05T20:26:42Z

...ensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ConfigProperties.java

+                Arrays.stream(keyValuePair.split("=", 2))
+                    .map(String::trim)
+                    .filter(s -> !s.isEmpty())
+                    .collect(Collectors.toList()))


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

jkwatson · 2021-01-05T20:37:37Z

...s/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ConfigurationException.java

+/** An exception that is thrown if the user-provided configuration is invalid. */
+public final class ConfigurationException extends RuntimeException {
+
+  private static final long serialVersionUID = 4717640118051490483L;


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

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

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

jkwatson · 2021-01-05T20:43:19Z

...oconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/MetricExporterConfiguration.java

+    switch (name) {
+      case "otlp":
+      case "otlp_metrics":
+        if (metricsAlreadyRegistered) {


it's a little annoying that this same check has to be done twice, but I don't see any easy away around it without some sort of ugly nesting of switch statements

jkwatson · 2021-01-05T20:47:07Z

...oconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/MetricExporterConfiguration.java

+        return true;
+      default:
+        return false;
+    }


I guess you could do:

switch (name) { case "otlp": case "otlp_metrics": case "prometheus": if (metricsAlreadyRegistered) { throw new ConfigurationException( "Multiple metrics exporters configured. Only one metrics exporter can be " + "configured at a time."); } if ("prometheus".equals(name)) { configurePrometheusMetrics(config, meterProvider); return true; } configureOtlpMetrics(config, meterProvider); return true; default: return false; }

but I don't know if that's really much better.

jkwatson · 2021-01-05T20:53:02Z

...gure/src/main/java/io/opentelemetry/sdk/autoconfigure/OpenTelemetrySdkAutoConfiguration.java

+    SdkTracerProvider tracerProvider =
+        TracerProviderConfiguration.configureTracerProvider(resource, exporterNames, config);
+
+    if (!exporterNames.isEmpty()) {


I really really don't like that the previous code mutates the set and we're relying on that fact here. That was super surprising to me...in fact, I was sure this line was a bug until I dug into the implementations.

Even if it ends up being uglier, I'd very strongly prefer it if we didn't have hidden side-effects in the code like this.

Yeah I mention it in a spec issue too :)

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

Do you see an alternative? Should I mutate a handledExportNames with .add instead?

You could return an updated copy of the set, maybe?

It's tricky since already returning a tracerprovider from one of them. It's a bit of duplication but I went with exposing the recognized names and checking eagerly instead of mutations.

jkwatson · 2021-01-05T20:56:19Z

...oconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/TracerProviderConfiguration.java

+      SpanExporter exporter = SpanExporterConfiguration.configureExporter(name, config);
+      if (exporter != null) {
+        spanExporters.add(exporter);
+        exporterNames.remove(name);


jkwatson · 2021-01-05T21:03:51Z

.../src/test/java/io/opentelemetry/sdk/autoconfigure/OpenTelemetrySdkAutoConfigurationTest.java

+        OpenTelemetrySdkAutoConfiguration.configureResource(
+            ConfigProperties.createForTest(
+                Collections.singletonMap("otel.resource.attributes", "telemetry.sdk.name=test")));
+    assertThat(resource.getAttributes().get(SemanticAttributes.TELEMETRY_SDK_NAME))


I didn't even realize that this was a thing that end-users could do. Is this desirable behavior?

I think we'll need it for service name? I don't think a user should do something bad but it seems to make sense semantically to prioritize them if they do go to the effort.

I don't know if it's generally desirable or not, but it certainly is the way it works today, so have a test for it is good. :)

jkwatson · 2021-01-05T21:05:53Z

...toconfigure/src/testConfigError/java/io/opentelemetry/sdk/autoconfigure/ConfigErrorTest.java

+  void globalOpenTelemetryWhenError() {
+    Logger logger = Logger.getLogger(GlobalOpenTelemetry.class.getName());
+    AtomicReference<LogRecord> logged = new AtomicReference<>();
+    Handler handler =


It'll be nice to convert to the logunit code!

… into autoconfigurre

jkwatson · 2021-01-06T16:48:18Z

...gure/src/main/java/io/opentelemetry/sdk/autoconfigure/OpenTelemetrySdkAutoConfiguration.java

+
+    Set<String> unrecognizedExporters = new LinkedHashSet<>(exporterNames);
+    unrecognizedExporters.removeAll(SpanExporterConfiguration.RECOGNIZED_NAMES);
+    unrecognizedExporters.removeAll(MetricExporterConfiguration.RECOGNIZED_NAMES);


this is a good solution. thanks!

jkwatson · 2021-01-06T16:49:47Z

...oconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/TracerProviderConfiguration.java

+          Double ratio = config.getDouble("otel.trace.sampler.arg");
+          if (ratio == null) {
+            throw new ConfigurationException(
+                "otel.trace.sampler=traceidratio but otel.trace.sampler.arg is not provided. "


stuff like this is a good argument for extracting the keys into constants, but that can be done as a separate step later, for sure.

jkwatson · 2021-01-06T16:52:19Z

...ensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ConfigProperties.java

+  }
+
+  // Visible for testing
+  @SuppressWarnings({"unchecked", "rawtypes"})


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

...ensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ConfigProperties.java

jkwatson

Thanks!

…k/autoconfigure/ConfigProperties.java Co-authored-by: John Watson <jkwatson@gmail.com>

… into autoconfigurre

Add autoconfiguration wrapper artifact

ba20433

anuraaga requested review from trask, Oberon00, jkwatson, bogdandrutu and iNikem December 23, 2020 08:40

anuraaga commented Dec 23, 2020

View reviewed changes

jkwatson reviewed Dec 23, 2020

View reviewed changes

build.gradle Show resolved Hide resolved

jkwatson reviewed Dec 23, 2020

View reviewed changes

Anuraag Agrawal added 3 commits December 24, 2020 14:40

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

78d46ea

… into autoconfigurre

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

3603143

… into autoconfigurre

WIP

0410fef

Anuraag Agrawal added 8 commits December 29, 2020 13:16

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

65c31d4

… into autoconfigurre

WIP

a72eee1

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

d696dac

… into autoconfigurre

WIP

c8c857c

WIP

e032f4c

WIP

7e0d223

Mostly done

8c670b4

Propagator classpath

c83836b

anuraaga commented Jan 4, 2021

View reviewed changes

Finish

7d92ee2

anuraaga requested review from pavolloffay, thisthat and tylerbenson as code owners January 5, 2021 04:36

jkwatson reviewed Jan 5, 2021

View reviewed changes

Anuraag Agrawal added 5 commits January 6, 2021 09:20

Cleanup

113951f

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

e27dc42

… into autoconfigurre

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

83a5bd8

… into autoconfigurre

Cleanup

a1eb942

Not visible

31853bc

jkwatson reviewed Jan 6, 2021

View reviewed changes

...ensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ConfigProperties.java Outdated Show resolved Hide resolved

jkwatson approved these changes Jan 6, 2021

View reviewed changes

Anuraag Agrawal and others added 3 commits January 7, 2021 11:40

Update sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sd…

5c386a4

…k/autoconfigure/ConfigProperties.java Co-authored-by: John Watson <jkwatson@gmail.com>

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

183cfdc

… into autoconfigurre

More merge

06048d4

anuraaga merged commit e749f2b into open-telemetry:master Jan 7, 2021

	* prefer to use system properties for configuration, but this may be useful to register components
	* prefer to use system properties or environment variables for configuration, but this may be useful to register components

Add autoconfiguration wrapper artifact #2401

Add autoconfiguration wrapper artifact #2401

Conversation

anuraaga commented Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

anuraaga commented Dec 23, 2020

marcingrzejszczak commented Dec 23, 2020

jkwatson commented Dec 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson commented Dec 23, 2020

codecov bot commented Dec 25, 2020 • edited Loading

Codecov Report

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson commented Jan 4, 2021

anuraaga commented Jan 4, 2021

anuraaga commented Jan 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson left a comment

Choose a reason for hiding this comment

anuraaga commented Dec 23, 2020 •

edited

Loading

codecov bot commented Dec 25, 2020 •

edited

Loading