[Backport to 0.34] Configuration do not cache sender instance

[ehsavoie]

Backport #685

• Adding a closed attribute on sender
• Sender configuration is resolving again is sender is closed.

Issue: #684
Signed-off-by: Emmanuel Hugonnet ehugonne@redhat.com

[codecov]

Codecov Report

Merging #686 into release-0.34 will decrease coverage by 0.01%.
The diff coverage is 100%.

@@                Coverage Diff                 @@
##             release-0.34     #686      +/-   ##
==================================================
- Coverage           89.13%   89.12%   -0.02%     
  Complexity            540      540              
==================================================
  Files                  68       68              
  Lines                1952     1950       -2     
  Branches              252      251       -1     
==================================================
- Hits                 1740     1738       -2     
  Misses                135      135              
  Partials               77       77

Impacted Files	Coverage Δ	Complexity Δ
.../src/main/java/io/jaegertracing/Configuration.java	95.47% <100%> (-0.04%)	44 <0> (ø)

---

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 3e20f62...82a2686. Read the comment docs.

[yurishkuro]

Sorry, this is not an acceptable change. It is changing the semantics of the Configuration class by removing the caching behavior.

[pavolloffay]

@yurishkuro see the comment in #685 (comment)

My idea is to cache the tracer but the Configuration.getTracerBuilder() would return a new instance of the builder with all parts. I cannot think of a reason why we should cache inner configuration objects.

[ehsavoie]

@yurishkuro so changing the configuration will return the cached tracer and not a tracer in sync with the configuration you are calling the getTracer from ? I find this counter intuitive. Also that means that you can get a closed tracer or as i found out a tracer with an underlying closed sender.

[yurishkuro]

@pavolloffay the getTracer() method no longer returns cached tracer, that is a breaking change. To my knowledge there was no other "caching" behavior, e.g. a sender is not cached, but can be provided externally as already built.

@ehsavoie this configuration mechanism was not designed to be "changed" after the tracer is created. Generally, a tracer is created only once per application. I don't understand what use case you have in mind in the ticket. "Reloading" the application would create a new Config object anyway.

[ehsavoie]

@yurishkuro the sender is being cached once it is resolved
I'm using the client in WildFly where the tracer is injected into a application but the configuration can be 'shared': for example to have several wars tracing to a single jaeger instance.

[ehsavoie]

@yurishkuro tbh I can leave the cached tracer in place if that's your concern, as long as the sender is not cached I can live with it since I'm using a tracerBuilder

[yurishkuro]

where the tracer is injected into a application but the configuration can be 'shared': for example to have several wars tracing to a single jaeger instance.

Shouldn't each application have a distinct service name, which necessitates a separate config?

And couldn't you simply clone the config object from a shared one before calling getTracer()?

[yurishkuro]

I'm ok with removing caching of the sender, it looks like the comment

// A custom sender set by our consumers. If set, nothing else has effect. Optional.

is misleading, since there is no setter for that private field.

[pavolloffay]

@pavolloffay the getTracer() method no longer returns cached tracer, that is a breaking change. To my knowledge there was no other "caching" behavior, e.g. a sender is not cached, but can be provided externally as already built.

I am saying that tracer instance returned by Configuration.getTracer() should be cached. But It does not make sense to cache Configuration.getTracerBuilder() - it already returns a new tracer builder but the class caches SenderConfiguration and other sub-configuration classes.

[pavolloffay]

@yurishkuro could you please have a look? It looks good to go.