Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Add possibility to specify serviceName with 'fromEnv' #510

Merged
merged 1 commit into from
Aug 7, 2018
+22 −14
Merged

Add possibility to specify serviceName with 'fromEnv' #510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions jaeger-core/src/main/java/io/jaegertracing/Configuration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,11 +186,15 @@ public Configuration(String serviceName) {
* @return Configuration object from environmental variables
*/
public static Configuration fromEnv() {
return new Configuration(getProperty(JAEGER_SERVICE_NAME))
.withTracerTags(tracerTagsFromEnv())
.withReporter(ReporterConfiguration.fromEnv())
.withSampler(SamplerConfiguration.fromEnv())
.withCodec(CodecConfiguration.fromEnv());
return Configuration.fromEnv(getProperty(JAEGER_SERVICE_NAME));
}

public static Configuration fromEnv(String serviceName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to add this if it is the only init-time required parameter. Otherwise I think the error handling should be moved to build method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the only required parameter.

return new Configuration(serviceName)
.withTracerTags(tracerTagsFromEnv())
.withReporter(ReporterConfiguration.fromEnv())
.withSampler(SamplerConfiguration.fromEnv())
.withCodec(CodecConfiguration.fromEnv());
}

public JaegerTracer.Builder getTracerBuilder() {
Expand Down
22 changes: 13 additions & 9 deletions jaeger-core/src/test/java/io/jaegertracing/ConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,11 @@
import io.jaegertracing.internal.samplers.RateLimitingSampler;
import io.jaegertracing.spi.Codec;
import io.jaegertracing.spi.Sampler;
import io.opentracing.noop.NoopTracerFactory;
import io.opentracing.propagation.Format;
import io.opentracing.propagation.Format.Builtin;
import io.opentracing.propagation.TextMap;
import io.opentracing.propagation.TextMapExtractAdapter;
import io.opentracing.propagation.TextMapInjectAdapter;
import io.opentracing.util.GlobalTracer;
import java.lang.reflect.Field;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
Expand Down Expand Up @@ -74,18 +71,25 @@ public void clearProperties() throws NoSuchFieldException, IllegalAccessExceptio
System.clearProperty(Configuration.JAEGER_PROPAGATION);

System.clearProperty(TEST_PROPERTY);

// Reset opentracing's global tracer
Field field = GlobalTracer.class.getDeclaredField("tracer");
field.setAccessible(true);
field.set(null, NoopTracerFactory.create());
}

@Test
public void testFromEnv() {
System.setProperty(Configuration.JAEGER_SERVICE_NAME, "Test");
assertNotNull(Configuration.fromEnv().getTracer());
assertFalse(GlobalTracer.isRegistered());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If removing from here, then should remove GlobalTracer completely from this test class (i.e. clearProperies()).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I removed it because I find it confusing that the test would make a relationship between fromEnv() and the GlobalTracer. If there's indeed a need to ensure the global tracer is not registered after fromEnv() is called, I can put it back.

}

@Test
public void testFromEnvWithExplicitServiceName() {
// prepare
String serviceName = "testFromEnvWithExplicitServiceName";
System.setProperty(Configuration.JAEGER_SERVICE_NAME, "not" + serviceName);

// test
JaegerTracer tracer = Configuration.fromEnv(serviceName).getTracer();

// check
assertEquals(serviceName, tracer.getServiceName());
}

@Test
Expand Down