Configuration class for BatchSpanProcessor

[thisthat]

This addresses #996 creating a configuration class for the BatchSpanProcessor.
The ConfigBuilder class contains the common code that will be shared with other configuration classes.

[jkwatson]

This PR proposes replacing Builders with Config classes. Any idea what set of SDK entities would end up being converted over to Config-based creation?

[bogdandrutu]

@jkwatson I think I posted in the issue what is the target

[bogdandrutu]

@thisthat any progress on this? I think we added some ideas in the comments.

[thisthat]

Sorry, I was pretty busy this week with internal stuff. I will work on this in the next days!

[codecov-io]

Codecov Report

Merging #1080 into master will increase coverage by 0.13%.
The diff coverage is 92.42%.

@@             Coverage Diff              @@
##             master    #1080      +/-   ##
============================================
+ Coverage     85.54%   85.68%   +0.13%     
  Complexity     1085     1085              
============================================
  Files           138      138              
  Lines          3993     4037      +44     
  Branches        356      366      +10     
============================================
+ Hits           3416     3459      +43     
+ Misses          435      433       -2     
- Partials        142      145       +3     

Impacted Files	Coverage Δ	Complexity Δ
...metry/exporters/jaeger/JaegerGrpcSpanExporter.java	59.72% <0.00%> (ø)	4.00 <0.00> (ø)
...elemetry/sdk/trace/export/BatchSpansProcessor.java	94.35% <93.84%> (+2.62%)	10.00 <2.00> (+1.00)
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java	84.15% <0.00%> (-1.10%)	45.00% <0.00%> (-1.00%)

---

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 b14a2dd...fc02ade. Read the comment docs.

[thisthat]

After 957ecc2, I have two questions:

1. Is it ok to normalize with lowercase and dot separation the lookup of variables? Currently, MY_ENV_VAR is the same as my.env.var which is the same as MY_env.VaR.

2. Since the configuration for other classes will look like the same, do we want to have a single configuration class that holds all configuration parameters of OTel?

[carlosalberto]

it ok to normalize with lowercase and dot separation the lookup of variables? ;)

I'd go with YES.

Since the configuration for other classes will look like the same, do we want to have a single configuration class that holds all configuration parameters of OTel?

Strong yes here - I think lots of these naming convention/config routines can be reused in other places. It can be done in a follow-up PR though.

[jkwatson]

Not an expert of Zipkin, but overall looks great @jkwatson You need to add the new artifact to all/build.gradle too, btw ;)

Not sure why this comment ended up over here, @carlosalberto

[bogdandrutu]

I really like the direction of this PR.

[jkwatson]

Is there a desire to have unified env vars across language implementations? If so, should we write a spec for these, for more widespread review?

[bogdandrutu]

@thisthat this PR is on you now, we need fixes for the round of comments we have for the moment.

[thisthat]

Is there a desire to have unified env vars across language implementations? If so, should we write a spec for these, for more widespread review?

I will open an issue in the spec after we are done with the configuration for all classes so I can provide the list of env vars we need. Regarding this topic, there is already an issue for setting the exporter host: open-telemetry/opentelemetry-specification#172

@bogdandrutu and @jkwatson, I have addressed your the suggestions, PTAL.

[bogdandrutu]

I think we are up for a good start. More refactoring will come when we change the next class.

[jkwatson]

Is there a desire to have unified env vars across language implementations? If so, should we write a spec for these, for more widespread review?

See open-telemetry/opentelemetry-specification#572

[jkwatson]

needs a rebase, but let's get this in and start the refactoring/cleanup

[carlosalberto]

As this PR looks ready to be merged: shall we create an issue to keep track of the upcoming configuration refactoring, so we can use this all over our SDK components?

[jkwatson]

As this PR looks ready to be merged: shall we create an issue to keep track of the upcoming configuration refactoring, so we can use this all over our SDK components?

I think the refactoring should come naturally by adding the rest of the configuration. I don't think we need a separate issue to track it.

[bogdandrutu]

@jkwatson what about my comments? Do we want to get them fixed?

[jkwatson]

@jkwatson what about my comments? Do we want to get them fixed?

Sure. I thought that since you had approved, that they had all been resolved. @thisthat could you respond to the last couple of @bogdandrutu 's comments?

[thisthat]

@jkwatson what about my comments? Do we want to get them fixed?

Sure. I thought that since you had approved, that they had all been resolved. @thisthat could you respond to the last couple of @bogdandrutu 's comments?

I totally agree with @bogdandrutu 's comment and I have addressed them!