Add OTLP protocol configurability to autoconfigure #3522
Conversation
|
Thanks for this @jack-berg Can you separate this into 2 PRs: 1 for the new experimental env vars, and 1 for the refactoring? |
Sure. Just to clarify, the preference is to add the new experimental env vars, with the repetitious reading of config properties in one PR. Then in a followup PR, abstract away the repetitious reading of config properties. |
Or, in the other order...whichever works best for you. :) |
8bb0ebb to
f0a88ce
Compare
|
I've rebased onto main which has the refactor, and added the |
...xtensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtil.java
Outdated
Show resolved
Hide resolved
...xtensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtil.java
Outdated
Show resolved
Hide resolved
| @@ -38,7 +40,7 @@ static SpanExporter configureExporter(String name, ConfigProperties config) { | |||
|
|
|||
| switch (name) { | |||
| case "otlp": | |||
| return configureOtlpSpans(config); | |||
| return configureOtlp(config); | |||
| builder::addHeader, | ||
| builder::setTimeout, | ||
| builder::setTrustedCertificates); | ||
| OtlpConfigUtil.configureOtlpExporterBuilder( |
There was a problem hiding this comment.
The OtlpConfigUtil calls here and in SpanExporterConfiguration make me wish for a Builder interface that would contain required 4 methods and allow us to remove the duplication. But this is out of scope of this change, so entirely up to you.
There was a problem hiding this comment.
I actually went down that road first and added the builder interface to :exporters:otlp:common. The problem was that I was stuck getting NoClassDefFoundError at runtime when the OtlpExporterBuilder interface is referenced. The pattern in the autoconfigure module of using ClasspathUtil.checkClassExists(...) seems to be deterministic at runtime in that java doesn't try to load the class until after the ClasspathUtil.checkClassExists call. But for some reason I couldn't figure out, that behavior wasn't the same with OtlpExporterBuilder.
| class OtlpConfigUtilTest { | ||
|
|
||
| @Test | ||
| void getOtlpProtocolDefault() { |
f0a88ce to
d35e4c3
Compare
Related to this open spec issue.
A note on the implementation: I was getting annoyed with how repetitive the configuration was for OTLP exporters. The extraction of config properties for OTLP grpc traces / metrics exporters is mostly the same. Adding support for protocol configurability of protocol means more copy / pasta or adding some abstraction. Went with adding an abstraction.
I initially added a
OtlpExporterBuilderinterface located in:exporters:otlp:commonwith methods common across all the OTLP exporters, but ran into all sorts ofNoClassDefFoundErrorerrors I couldn't get past. The solution I landed is to have a method that accepts a bunch of consumers which are invoked after config properties are extracted. Its pretty clean IMO, with one downside being a method that accepts 6 arguments.