Polish configuration API #411

pavolloffay · 2018-05-15T15:39:09Z

Resolves #394

there are getters for all configuration properties
codec configuration can be created programmatically
allow overriding service name

Configuration can be created programmatically o via env. If it is created via env all properties can be changed programmatically.

Signed-off-by: Pavol Loffay ploffay@redhat.com

codecov · 2018-05-15T15:45:40Z

Codecov Report

Merging #411 into master will increase coverage by 0.09%.
The diff coverage is 92.3%.

@@             Coverage Diff              @@
##             master     #411      +/-   ##
============================================
+ Coverage     82.24%   82.33%   +0.09%     
- Complexity      487      490       +3     
============================================
  Files            66       66              
  Lines          2033     2044      +11     
  Branches        247      247              
============================================
+ Hits           1672     1683      +11     
+ Misses          280      279       -1     
- Partials         81       82       +1

Impacted Files	Coverage Δ	Complexity Δ
.../src/main/java/io/jaegertracing/Configuration.java	`81.56% <92.3%> (+0.97%)`	`37 <3> (+4)`	⬆️
...aegertracing/samplers/RemoteControlledSampler.java	`77.77% <0%> (-1.02%)`	`17% <0%> (-1%)`

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 6f75dfe...ec99621. Read the comment docs.

aaronkorver · 2018-05-15T15:47:51Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

+    public CodecConfiguration() {
+    }
+
+    public CodecConfiguration withCodec(Format<?> format, Codec<TextMap> codec) {


aaronkorver · 2018-05-15T15:52:52Z

A note, not necessary on this PR as I can open a separate issue, the withTags() would be great if there was exposed a withTagsFromEnv() as well that could use the private method that is currently in the config.

pavolloffay · 2018-05-15T16:09:13Z

@aaronkorver you can call c = Configuration.fromEnv() then c.getTags() and then c.withTags(tags) to fully manipulate with tags.

aaronkorver · 2018-05-15T16:35:07Z

right, but I can't call the parser function in Configuration as it is private right now. I have some teams that want to take advantage of the JAEGER_TAGS environment. My team has been providing a library for tracing and want them to use it as well, but have basically cut/paste that function into our library so we can configure certain things from the ENV, but override other pieces. Hence the suggestion to do withTagsFromEnv()

pavolloffay · 2018-05-15T16:49:07Z

I am not sure whether method c.withTagsFromEnv() makes sense as the tags from env are obtained during Configuration.fromEnv().

We could perhaps expose map = Configuration.tracerTagsFromEnv() , then you can get tags from env and programmatically mix things together. Oh wait actually right now you can call fromEnv() and get parsed tags from the configuration.

pavolloffay · 2018-05-15T17:47:41Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

@@ -280,8 +280,14 @@ public void setStatsFactory(StatsFactory statsFactory) {
  /**
   * @param metricsFactory the MetricsFactory to use on the Tracer to be built
   */
-  public void withMetricsFactory(MetricsFactory metricsFactory) {
+  public Configuration withMetricsFactory(MetricsFactory metricsFactory) {


This shouldn't break clients

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

aaronkorver · 2018-05-15T18:12:15Z

So are you recommending that you should first do myConfig = Configuration.fromEnv() and then change things within myConfig? The unfortunate part on that is the getTracerTags() returns an unmodifiableMap() that I can't merge things.

pavolloffay · 2018-05-16T07:40:20Z

So are you recommending that you should first do myConfig = Configuration.fromEnv() and then change things within myConfig

Yes, the map is unmodifiable for a purpose, you can do m = new HasMap<>(unmodifiableMap)

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

jpkrohling

Apart from a small request, this LGTM

jpkrohling · 2018-05-16T10:36:06Z

jaeger-core/src/test/java/io/jaegertracing/ConfigurationTest.java

@@ -328,6 +333,14 @@ public void testNoServiceName() {
    new Configuration(null);
  }

+  @Test
+  public void testOverrideServiceName() {


Can you add a test using withServiceName(null), and another with withServiceName("")? This should document what is expected to happen in such case.

It's not strictly related to this PR, but I am happy to add it.

Test for null service name is already there, I have added only for empty case.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

jpkrohling · 2018-05-16T12:17:17Z

jaeger-core/src/test/java/io/jaegertracing/ConfigurationTest.java

@@ -328,6 +333,19 @@ public void testNoServiceName() {
    new Configuration(null);
  }

+  @Test(expected = RuntimeException.class)
+  public void testEmptyServiceName() {
+    new Configuration("");


I actually thought about having the null overriding a valid existing name:

new Configuration("Test").withServiceName(null);

This way, you'd be documenting the expected behavior for this scenario, as it might be ambiguous. This is not required, so, I'm giving my +1 to this PR.

The check is also in tracer constructor, but in this case we wanted to fail before tracer-init time

ghost assigned pavolloffay May 15, 2018

ghost added the review label May 15, 2018

pavolloffay mentioned this pull request May 15, 2018

Revisit Configuration API #394

Closed

aaronkorver reviewed May 15, 2018

View reviewed changes

pavolloffay commented May 15, 2018

View reviewed changes

pavolloffay added 2 commits May 15, 2018 19:50

Polish configuration API

c14364f

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Add more tests, fix empty map

17ea0a1

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the polish-configuration-api branch from d3e436e to 17ea0a1 Compare May 15, 2018 17:52

pavolloffay requested a review from jpkrohling May 16, 2018 08:10

Test default codecs

43f8886

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

jpkrohling reviewed May 16, 2018

View reviewed changes

Add empty service name test

ec99621

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

jpkrohling approved these changes May 16, 2018

View reviewed changes

pavolloffay merged commit 818d741 into jaegertracing:master May 16, 2018

ghost removed the review label May 16, 2018

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polish configuration API #411

Polish configuration API #411

pavolloffay commented May 15, 2018 •

edited

Loading

codecov bot commented May 15, 2018 •

edited

Loading

aaronkorver May 15, 2018

aaronkorver commented May 15, 2018

pavolloffay commented May 15, 2018

aaronkorver commented May 15, 2018

pavolloffay commented May 15, 2018

pavolloffay May 15, 2018

aaronkorver commented May 15, 2018

pavolloffay commented May 16, 2018

jpkrohling left a comment

jpkrohling May 16, 2018

pavolloffay May 16, 2018

jpkrohling May 16, 2018

pavolloffay May 16, 2018

Polish configuration API #411

Polish configuration API #411

Conversation

pavolloffay commented May 15, 2018 • edited Loading

codecov bot commented May 15, 2018 • edited Loading

Codecov Report

aaronkorver May 15, 2018

Choose a reason for hiding this comment

aaronkorver commented May 15, 2018

pavolloffay commented May 15, 2018

aaronkorver commented May 15, 2018

pavolloffay commented May 15, 2018

pavolloffay May 15, 2018

Choose a reason for hiding this comment

aaronkorver commented May 15, 2018

pavolloffay commented May 16, 2018

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling May 16, 2018

Choose a reason for hiding this comment

pavolloffay May 16, 2018

Choose a reason for hiding this comment

jpkrohling May 16, 2018

Choose a reason for hiding this comment

pavolloffay May 16, 2018

Choose a reason for hiding this comment

pavolloffay commented May 15, 2018 •

edited

Loading

codecov bot commented May 15, 2018 •

edited

Loading