Add possibility to specify serviceName with 'fromEnv' #510
Conversation
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #510 +/- ##
============================================
+ Coverage 88.2% 88.31% +0.11%
- Complexity 498 499 +1
============================================
Files 65 65
Lines 1848 1849 +1
Branches 239 239
============================================
+ Hits 1630 1633 +3
+ Misses 141 139 -2
Partials 77 77
Continue to review full report at Codecov.
|
| @@ -85,7 +85,19 @@ public void clearProperties() throws NoSuchFieldException, IllegalAccessExceptio | |||
| public void testFromEnv() { | |||
| System.setProperty(Configuration.JAEGER_SERVICE_NAME, "Test"); | |||
| assertNotNull(Configuration.fromEnv().getTracer()); | |||
| assertFalse(GlobalTracer.isRegistered()); | |||
There was a problem hiding this comment.
If removing from here, then should remove GlobalTracer completely from this test class (i.e. clearProperies()).
There was a problem hiding this comment.
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.
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling
force-pushed
the
AddFromEnvWithServiceName
branch
from
557d69d to
a8c8225
Compare
| return Configuration.fromEnv(getProperty(JAEGER_SERVICE_NAME)); | ||
| } | ||
|
|
||
| public static Configuration fromEnv(String serviceName) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's the only required parameter.
Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de
Which problem is this PR solving?
withmethods (withTracerTagsbeing the most problematic one, IMO).Setting/clearing a system property might be trivial for single tenant JVM instances, but it's a problem when the JVM has multiple tenants, which is usually the case for application servers like WildFly.
Short description of the changes
Configuration.fromEnv(String)method based on the existingConfiguration.fromEnv()oneConfiguration.fromEnv()to use the new method, passingJAEGER_SERVICE_NAMEas theserviceName.