Refactor public builder API #346
Conversation
ghost
assigned 
| @@ -449,6 +453,10 @@ public Scope startActive(boolean finishSpanOnClose) { | |||
| private ScopeManager scopeManager = new ThreadLocalScopeManager(); | |||
| private BaggageRestrictionManager baggageRestrictionManager = new DefaultBaggageRestrictionManager(); | |||
|
|
|||
| public Builder() { | |||
There was a problem hiding this comment.
maybe Tracer.builder(serviceName) instead? What are the conventions in Java these days? I personally prefer static methods to constructors.
| return new Tracer(this.serviceName, reporter, sampler, registry, clock, metrics, tags, | ||
| if (serviceName == null) { | ||
| serviceName = "unknown"; | ||
| log.warn("Service name hasn't been configured, using " + serviceName + " instead"); |
There was a problem hiding this comment.
What is the argument for allowing "unknown"? Service name is a fundamental concept in Jaeger, I would rather require it in the builder() func.
There was a problem hiding this comment.
+1 there si a comment in a PR description about this
| } | ||
| Reporter reporter = new RemoteReporter(sender, flushInterval, maxQueueSize, metrics); | ||
| if (withLogging) { | ||
| reporter = new CompositeReporter(new LoggingReporter(), reporter); |
There was a problem hiding this comment.
This is one reason why the separation of configuration exists. I think it breaks encapsulation if RemoteReporter suddenly becomes aware of things like logging reporter.
There was a problem hiding this comment.
It can be removed from here, I think it's better to add it directly in TracerBuilder
| @@ -29,4 +29,8 @@ | |||
| * Release any resources used by the sampler. | |||
| */ | |||
| void close(); | |||
|
|
|||
| interface Builder { | |||
There was a problem hiding this comment.
The reason is mentioned in PR description. Some samplers are easy to create, however some (like RemoteControlledSampler) has several configuration options therefore there is a generic builder.
There was a problem hiding this comment.
when someone creates a concrete sampler/reporter, they are dealing with a concrete class. If it happened to have a builder, it would be specific to that class. So I am not seeing the value of a common builder interface, unless the intention is to pass builders around.
| * This constructor expects Jaeger running running on {@value #DEFAULT_AGENT_UDP_HOST} and port {@value #DEFAULT_AGENT_UDP_COMPACT_PORT} | ||
| */ | ||
| public UdpSender() { | ||
| this(DEFAULT_AGENT_UDP_HOST, DEFAULT_AGENT_UDP_COMPACT_PORT, 0); |
There was a problem hiding this comment.
It is, look how it was called from the configuration class. I don't like that default values are defined in configuration classes.
| @@ -175,10 +175,12 @@ | |||
| */ | |||
| private Tracer tracer; | |||
|
|
|||
| @Deprecated | |||
There was a problem hiding this comment.
does this still allow populating Configuration object from a config file?
There was a problem hiding this comment.
Ignore this, I want to say that only Tracer.Builder builder = Configuration.fomEnv() should be used. See PR description
|
@yurishkuro thanks for the review. Do you think we want to proceed with this approach? Don't do nit pitching, just see the overall approach described in the first comment in this PR. |
|
I do like the idea of coalescing defaults into one place. Whether that's configs or builders - no strong opinion. The original issue was about config classes being difficult to construct programmatically. I think this PR's approach simply bypasses that problem by offering construction via builders directly in the components, i.e. if you're constructing programmatically you don't need to use the configs. +1 to that. My only additional requirement is that changes to configs should be kept backwards compatible (deprecated - fine). The primary use case for config class at Uber is when it is populated from a config file, so that should still work (not sure if we have tests for that, maybe in dw module). |
|
I won't remove config classes in this PR. internals in config classes will use these new builders. I will iterate over tomorrow to the final state. The high level idea of this PR is to move builders to impl classes so impl classes are responsible of creating. Then there is a configuration layer which just maps configfiles/env/props to these builders.
Are is it possible to share this? e.g. if there is a code which loads yaml/json and creates Jaeger tracer that could be moved here I guess. |
Codecov Report
@@ Coverage Diff @@
## master #346 +/- ##
============================================
- Coverage 84.17% 83.93% -0.24%
- Complexity 583 590 +7
============================================
Files 92 92
Lines 2268 2390 +122
Branches 266 272 +6
============================================
+ Hits 1909 2006 +97
- Misses 257 281 +24
- Partials 102 103 +1
Continue to review full report at Codecov.
|
|
I have moved all default values to impl classes. To summarize this:
Here is a code snippet: // Tracer Builder directly
Tracer tracer = new Tracer.Builder("name")
.withReporter(new Builder()
.withSender(new HttpSender("http://localhost:8080"))
.withFlushInterval(100)
.build())
.withStatsReporter(new NullStatsReporter())
.withSampler(new RemoteControlledSampler.Builder("name")
.withInitialSampler(new ConstSampler(true))
.build())
.build();
// Configuration from env
System.setProperty(Configuration.JAEGER_SERVICE_NAME, "name");
Configuration configuration = Configuration.fromEnv();
configuration.getReporterConfig()
.getSenderConfiguration()
.getBuilder()
.endpoint("http://jaegereverywhere.com");
configuration.getTracer();
// Configuration directly
new Configuration("name")
.reporterConfiguration(new ReporterConfiguration()
.senderConfiguration(new SenderConfiguration.Builder()
.endpoint("http://jaegereverywhere.com")
.build()))
.getTracer();
} |
|
@yurishkuro @vprithvi @jpkrohling @objectiser could you please have a look? PR is not ready to merge, but the overall ideas shouldn't change |
There was a problem hiding this comment.
general comment, just my preference but what if we make all new builder constructors private and instead have a static builder() method on the respective class, e.g. Tracer.builder(), RepoteReporter.builder()? I think it improves readability of initialization code.
| @@ -263,56 +261,79 @@ public void setStatsFactory(StatsFactory statsFactory) { | |||
| this.statsFactory = statsFactory; | |||
| } | |||
|
|
|||
| public Configuration reporterConfiguration(ReporterConfiguration reporterConfig) { | |||
There was a problem hiding this comment.
nit: naming not consistent with setStatsFactory or with builders. Any reason not to converge on withXyz naming?
nit2: Configuration suffix can probably be dropped
There was a problem hiding this comment.
I think we can, but there will be more deprecated methods.
| } | ||
| }; | ||
| } | ||
|
|
||
| public static class Builder { |
There was a problem hiding this comment.
can we deprecate this class and just use SenderConfiguration directly?
There was a problem hiding this comment.
I am definitely +1 on that, I will try to do it
| stringOrDefault(this.agentHost, UdpSender.DEFAULT_AGENT_UDP_HOST), | ||
| numberOrDefault(this.agentPort, UdpSender.DEFAULT_AGENT_UDP_COMPACT_PORT).intValue(), | ||
| stringOrDefault(this.builder.agentHost, UdpSender.DEFAULT_AGENT_UDP_HOST), | ||
| numberOrDefault(this.builder.agentPort, UdpSender.DEFAULT_AGENT_UDP_COMPACT_PORT).intValue(), |
There was a problem hiding this comment.
should the defaults be moved into respective builders, like UdpSender.Builder?
There was a problem hiding this comment.
All defaults have been moved to impl classes
| this.sampler = sampler; | ||
|
|
||
| public Builder(String serviceName) { | ||
| this.withServiceName(serviceName); |
There was a problem hiding this comment.
Q: what is the scenario where withServiceName() method is needed instead of providing the name to the ctor?
There was a problem hiding this comment.
Not really, It can be removed.
| .withMetrics(metrics) | ||
| .build(); | ||
| } | ||
| if (withLoggingSpans) { |
There was a problem hiding this comment.
copying from other PR: I prefer not to have this option on the Tracer.Builder, it creates unnecessary coupling with CompositeReporter, LoggingReporter, etc. This feature can be left to Config classes only.
There was a problem hiding this comment.
I would recommend to people start using Tracer.Builder When they use programmatic API. I think it's useful to have it here directly. I am not sure if the composite reporter is easy enough to find/use.
| @@ -24,4 +24,8 @@ | |||
| void report(Span span); | |||
|
|
|||
| void close(); | |||
|
|
|||
| interface Builder<T extends Reporter> { | |||
There was a problem hiding this comment.
I do not believe this interface is needed. No class actually uses it.
| @@ -29,4 +29,8 @@ | |||
| * Release any resources used by the sampler. | |||
| */ | |||
| void close(); | |||
|
|
|||
| interface Builder<T extends Sampler> { | |||
There was a problem hiding this comment.
similarly, this is never used on the left hand side.
| @@ -23,4 +23,8 @@ | |||
| int flush() throws SenderException; | |||
|
|
|||
| int close() throws SenderException; | |||
|
|
|||
| interface Builder<T extends Sender> { | |||
| @@ -490,15 +553,15 @@ public String getAgentHost() { | |||
| return null; | |||
| } | |||
|
|
|||
| return this.senderConfiguration.agentHost; | |||
| return this.senderConfiguration.builder.agentHost; | |||
There was a problem hiding this comment.
Possibly we should deprecate these accessors (getAgentHost/getAgentPort) as looks like they are only used in tests?
There was a problem hiding this comment.
I was thinking about this, We definitely need accessors like Configuration.getReporterConfiguration then getSenderConfiguration to be able to override sender without re-creating other configs.
I am wondering whether there is an use case when app gets the configuration from env and then changes behaviour - e.g. if used endpoint it will get it and override to https for example.
There was a problem hiding this comment.
Don't think we should be supporting such use cases - but if someone wanted to make a case for it later, it could be discussed then.
| @Override | ||
| public RemoteReporter build() { | ||
| if (sender == null) { | ||
| sender = new UdpSender(UdpSender.DEFAULT_AGENT_UDP_HOST, UdpSender.DEFAULT_AGENT_UDP_COMPACT_PORT, 0); |
There was a problem hiding this comment.
Should just use the default constructor.
| OkHttpClient.Builder clientBuilder = new OkHttpClient.Builder(); | ||
| if (authInterceptor != null) { | ||
| clientBuilder.addInterceptor(authInterceptor); | ||
|
|
Can do, we can talk about this cosmetics at the end. |
pavolloffay
force-pushed
the
refactor-builders
branch
2 times, most recently
from
581d360
to
fc17680
Compare
|
I was able to rename conf method to |
| public void setStatsFactory(StatsFactory statsFactory) { | ||
| this.statsFactory = statsFactory; | ||
| } | ||
|
|
||
| public Configuration withStatsFactory(StatsFactory statsFactory) { |
| } | ||
|
|
||
| /** | ||
| * Holds the configuration related to the sender. A sender can be a {@link HttpSender} or {@link UdpSender} | ||
| * | ||
| */ | ||
| @Getter |
There was a problem hiding this comment.
looks like a breaking change
There was a problem hiding this comment.
Added back, although I think this was added only for tests by @jpkrohling. I don't think we had a requirement of exposing it.
pavolloffay
changed the title
pavolloffay
force-pushed
the
refactor-builders
branch
2 times, most recently
from
a950d09
to
395e90a
Compare
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
pavolloffay
force-pushed
the
refactor-builders
branch
from
395e90a
to
d94ed34
Compare
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
There was a problem hiding this comment.
@pavolloffay do you want to ramp up the coverage back into green? E.g. by simply invoking all the deprecated constructors.
|
@yurishkuro yes, that is what I am trying. I will merge on green |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Codecov Report
@@ Coverage Diff @@
## master #346 +/- ##
============================================
+ Coverage 84.17% 84.54% +0.37%
- Complexity 583 591 +8
============================================
Files 92 92
Lines 2268 2388 +120
Branches 266 272 +6
============================================
+ Hits 1909 2019 +110
- Misses 257 270 +13
+ Partials 102 99 -3
Continue to review full report at Codecov.
|
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
| } | ||
|
|
||
| public Map<String, String> getTracerTags() { | ||
| return tracerTags; |
There was a problem hiding this comment.
This should be wrapped in a Collections#unmodifiableMap.
| } | ||
|
|
||
| public Configuration withTracerTags(Map<String, String> tracerTags) { | ||
| this.tracerTags = tracerTags; |
There was a problem hiding this comment.
You should use a new map, like new HashMap(tracerTags), so that the caller won't change your copy of the map.
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Resolves #258 #44
Supersedes #336
The idea is to use
Configurationonly for creatingTracer.Builderfrom environmental variables (or in the future from whatever source).Reporter, Sender, Sampler classes are arbitrary difficult to construct therefore they might implement a builder interface defined in main Reporter, Sender, Sampler interface class.
Other high-level concepts:
unknownand sends data to the local agent.This is WIP without javadoc, tests. Please just look at the overall approach whether it is something we want.