For SpanProcessor/SpanExporter/MetricProducer/MetricExporter use Config

[bogdandrutu]

This is a proposal to use a Config object instead of Builders for all the implementations of these classes. This will allow us to have a "getFromEnv" on all these Configs, probably a "getDefault" as well.

This will be easier to initialize compared with Builders in auto-instrumentation as well as when dealing with initializing a pipeline like SpanProcessor -> SpanExporter.

/cc @trask @tylerbenson - what do you think about this from the auto-instrumentation perspective.

[jkwatson]

My only concern is we lose the rich semantics of the naming in the builder methods. Perhaps we have specific Config extension classes to provide the naming?

[bogdandrutu]

I would suggest like this:

class BatchSpansProcessor {

  BatchSpansProcessor create(Config config);

  // Immutable 
  public static Config {
     boolean getReportOnlySampled();
     long getScheduleDelayMillis();
     long getExporterTimeoutMillis();
     int getMaxQueueSize();
     int getMaxExportBatchSize();

     public static Config fromEnv();

     public static Config getDefault();

     // Looks for the same env variables but in the given map not in the env variables.
     // Can be used by the auto-config framework.
     // Not sure if this helps, but it is an idea on how we can extend this.
     public static Config fromConfigMap(Map<String, String> configMap);

     public static Builder {
        // Setters for all the properties.
     }
  }
}

[jkwatson]

I like this suggestion quite a bit. I think it'll tighten up our config story a lot. 👍

[carlosalberto]

I'm fine with this 👍

[pavolloffay]

+1 on the proposal and It's great to see this landing in OTEL.

The configuration property names should be defined at the spec level to make consistent across SDKs.

There is one missing feature in the API, sometimes it is useful to change the configuration after it is created from the environment. Maybe fromEnv could return the builder instead and not an immutable object.

[Oberon00]

I would add applyEnv as an in-place method to the builder.

[tylerbenson]

@bogdandrutu on your proposal, how would it work if you want most of your configs to come from a default or env, but override one or two of the settings? Perhaps better to have those methods return a builder that can be modified before constructing the immutable config? (Duplicating those methods on the builder would also be acceptable.

[tylerbenson]

Also, how do people feel about accepting config via system properties in addition to env vars?

[carlosalberto]

how do people feel about accepting config via system properties in addition to env vars?

I'd strongly vow for supporting system properties too.

[carlosalberto]

For prior art: in the SpecialAgent, we first get env vars, and then proceed to look for them as system properties, in which case the values get overridden.

[jkwatson]

This is a nice pattern to support both, quite tersely:
System.getProperty(key, System.getenv(key))

[trask]

Thanks @bogdandrutu, this sounds great from an auto-instrumentation perspective! We need to give auto-instrumentation users some way to configure the SDK, and it would be really nice if we can just delegate this configuration to the SDK itself.

Also, I'm a fan of env vars as the most basic means of external configuration since those tend to be supported in the widest number of compute environments (and I have no objection to also having system properties support).

[pavolloffay]

This is a nice pattern to support both, quite tersely:
System.getProperty(key, System.getenv(key))

+1 This is already what Jaeger exporter configuration does https://github.com/open-telemetry/opentelemetry-java/blob/master/exporters/jaeger/src/main/java/io/opentelemetry/exporters/jaeger/JaegerGrpcSpanExporter.java#L240

[Oberon00]

Note that this also calls getenv when the system property is found. AFAIK, getenv is very cheap at least on OpenJDK/Oracle based JVMs though.

[bogdandrutu]

@Oberon00 or @thisthat anyone interested to work on this?

[thisthat]

@bogdandrutu I can take this!

[bogdandrutu]

@thisthat please pick one (I propose BatchSpanProcessor) and start a PR where we can discuss the overall design. Then we can apply the same changes to others. Does it make sense?

[thisthat]

@bogdandrutu yes, it totally makes sense! :)

[iNikem]

I am very confused about this task. PR #1080 added new Config classes as this task requested, but then PR #1217 has removed them again. Including the very convenient method Config. loadFromDefaultSources: https://github.com/open-telemetry/opentelemetry-java/pull/1080/files#diff-9563795c03b91d8c3d1aabf8cccdcc54R472 .

What is the "official" way now to configure span processor from system properties/env variables? @bogdandrutu @thisthat

[thisthat]

The task was proposing a way to configure from system and environment variables different parts of the SDK. Since the classes have already a builder, we introduce such functionality there without increasing the API surface.
If you think we should expose also a newBuilderFromDefaultSources() method, I can provide such enhancement.

[iNikem]

So it is expected now that every configuration source (env/sysprops) should be consulted manually by whoever creates a processor/exporter? By calling BatchSpanProcessor.newBuilder(exporter).readEnvironmentVariables().readSystemProperties().build() ?

[thisthat]

Exactly