Initial implementation of a TracerResolver for Jaeger #175

objectiser · 2017-05-10T07:58:47Z

This TracerResolver implementation uses the ConstSampler and NullStatsReporter. Configuration for other implementations will be provided as separate PRs. This PR is intended to provide a basic resolver implementation.

codecov-io · 2017-05-10T08:08:53Z

Codecov Report

Merging #175 into master will increase coverage by 0.91%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #175      +/-   ##
============================================
+ Coverage     79.66%   80.58%   +0.91%     
- Complexity      447      460      +13     
============================================
  Files            76       77       +1     
  Lines          1751     1782      +31     
  Branches        205      208       +3     
============================================
+ Hits           1395     1436      +41     
+ Misses          267      255      -12     
- Partials         89       91       +2

Impacted Files	Coverage Δ	Complexity Δ
...e/src/main/java/com/uber/jaeger/Configuration.java	`83.63% <100%> (+18.2%)`	`23 <10> (+11)`	⬆️
...er/jaeger/tracerresolver/JaegerTracerResolver.java	`100% <100%> (ø)`	`2 <2> (?)`

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 2603cc7...18fcf32. Read the comment docs.

mabn · 2017-05-10T09:16:32Z

jaeger-tracerresolver/README.md

@@ -0,0 +1,31 @@
+# Jaeger Tracer Resolver


It would be good to have here a short comment describing what it is

pavolloffay · 2017-05-10T10:31:56Z

jaeger-tracerresolver/build.gradle

+jar {
+    from sourceSets.main.output
+    manifest {
+        attributes('Implementation-Title': 'jaeger-tracerresolver', 'Implementation-Version': project.version)


Couldn't be this added to allProjects in root script?

It's repeated in every module.

If its possible then sounds like a good idea, but outside the remit of this PR.

pavolloffay · 2017-05-10T10:36:05Z

jaeger-tracerresolver/build.gradle

+    signature 'org.codehaus.mojo.signature:java16:1.1@signature'
+}
+
+sourceSets {


Can be this boilerplate avoided? I think it's redundant as we are sticking to default file hierarchy.

I know this is repeated over and over it other modules, but should be used only in places where it's required to override a default conventions.

As above, this should probably be discussed as a separate issue and applied to all artifacts.

The previous maybe yes. This can be done independently it does not affect other modules.

Yep that works.

pavolloffay · 2017-05-10T10:37:06Z

jaeger-tracerresolver/src/main/java/com/uber/jaeger/tracerresolver/JaegerTracerResolver.java

+@Slf4j
+public class JaegerTracerResolver extends TracerResolver {
+
+  static final String JAEGER_PREFIX = "JAEGER_";


these should be public and with javadoc

They are only used by the resolver.

They are used outside of this module. Consumers set these when starting instrumented app, so are public. Somebody could use them programmatically. Readmes might get outdated, therefore having javadoc is a plus.

pavolloffay · 2017-05-10T10:39:55Z

...er-tracerresolver/src/test/java/com/uber/jaeger/tracerresolver/JaegerTracerResolverTest.java

+    while (iter.hasMoreElements()) {
+      String propName = (String)iter.nextElement();
+      if (propName.startsWith(JaegerTracerResolver.JAEGER_PREFIX)) {
+        System.clearProperty(propName);


nit: clear only properties set by this test

…th jdoc, explicit clearing of properties

pavolloffay · 2017-05-10T12:01:06Z

...er-tracerresolver/src/test/java/com/uber/jaeger/tracerresolver/JaegerTracerResolverTest.java

+  @Before
+  public void clearProperties() {
+    // Explicitly clear all TracerResolver properties
+    System.clearProperty(JaegerTracerResolver.JAEGER_AGENT_UDP_HOST);


This does not clear properties set after this class. We could get all props at @BeforeClass and set them back at @After to keep all tests and class isolated.

yurishkuro · 2017-05-10T16:44:02Z

jaeger-tracerresolver/src/main/java/com/uber/jaeger/tracerresolver/JaegerTracerResolver.java

+  }
+
+  protected static Reporter getReporter() {
+    return new RemoteReporter(getSender(),


Could we avoid writing this code and instead use the Configuration class to create the tracer? Here we just need to set the parameters on the respective Config classes, but leave the init logic to one place in the main config.

pavolloffay · 2017-05-10T16:53:40Z

jaeger-tracerresolver/README.md

+Property | Default | Description
+--- | --- | ---
+JAEGER_SERVICE_NAME | _none_ | The service name (must be defined)
+JAEGER_AGENT_UDP_MAX_PACKET_SIZE | 65000 | The maximum packet size when communicating with the agent via UDP


These default values can get quickly outdated.

agreed - better to just say required/optional. The defaults will be applied by Configuration class.

ok will update

…efault values from the README.

yurishkuro

lgtm, but perhaps we should move the init portion to Configuration class?

objectiser · 2017-05-12T07:36:51Z

@yurishkuro Just to be clear - move everything to the Configuration class, with environment var based config via a default constructor and then the TracerResolver simply use the default constructor and return the tracer?

yurishkuro · 2017-05-12T16:14:31Z

move all config-related code to Configuration (env var declaration, instantiating config objects)
but not to default constructor, that would not be backwards compatible - to a static method Configuration.fromEnv(): Configuration

yurishkuro · 2017-05-12T16:51:49Z

jaeger-core/src/main/java/com/uber/jaeger/Configuration.java

+        getSamplerConfiguration(), getReporterConfiguration());
+  }
+
+  protected static ReporterConfiguration getReporterConfiguration() {


not private?

nit: getReporterConfigurationFromEnv

yurishkuro · 2017-05-12T16:57:12Z

jaeger-tracerresolver/README.md

+
+## Configuration Options
+
+The following configuration properties can be provided either as an environment variable or system property.


Should we move these to the README in core as well? We can link to that file from here.

Right now the main README only suggests

Configuration config = new Configuration("myServiceName", null, null);

for production. I would add a sub-section there "Configuration via Environment" with what we have here. And it would also make sense to add yet another subsection that it's possible to init via TracerLoader, and link to here.

Sounds good - I'll have to look at this next week.

…tifact's README and cross reference

Initial implementation of a TracerResolver for Jaeger

a40e73d

mabn reviewed May 10, 2017

View reviewed changes

Add description to the readme

1a8bf4d

pavolloffay reviewed May 10, 2017

View reviewed changes

Updates following review - remove sourceset, make constants public wi…

3f393a2

…th jdoc, explicit clearing of properties

pavolloffay reviewed May 10, 2017

View reviewed changes

Clear properties before and after test

1120eae

yurishkuro reviewed May 10, 2017

View reviewed changes

pavolloffay reviewed May 10, 2017

View reviewed changes

Changed resolver to use existing Configuration class and remove the d…

15b6e71

…efault values from the README.

yurishkuro reviewed May 11, 2017

View reviewed changes

Moved config init from env to Configuration class

18fcf32

yurishkuro reviewed May 12, 2017

View reviewed changes

yurishkuro approved these changes May 12, 2017

View reviewed changes

Move the documentation of the configuration properties to the core ar…

c6139ee

…tifact's README and cross reference

yurishkuro approved these changes May 14, 2017

View reviewed changes

yurishkuro merged commit 917dff5 into jaegertracing:master May 14, 2017

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

Initial implementation of a TracerResolver for Jaeger #175

Initial implementation of a TracerResolver for Jaeger #175

objectiser commented May 10, 2017

codecov-io commented May 10, 2017 •

edited

Loading

mabn May 10, 2017 •

edited

Loading

pavolloffay May 10, 2017

pavolloffay May 10, 2017

objectiser May 10, 2017

pavolloffay May 10, 2017

objectiser May 10, 2017

pavolloffay May 10, 2017

objectiser May 10, 2017

pavolloffay May 10, 2017 •

edited

Loading

objectiser May 10, 2017

pavolloffay May 10, 2017

pavolloffay May 10, 2017

pavolloffay May 10, 2017

yurishkuro May 10, 2017

pavolloffay May 10, 2017

yurishkuro May 10, 2017

objectiser May 11, 2017

yurishkuro left a comment

objectiser commented May 12, 2017

yurishkuro commented May 12, 2017

yurishkuro May 12, 2017

yurishkuro May 12, 2017

objectiser May 12, 2017


		## Configuration Options

		The following configuration properties can be provided either as an environment variable or system property.

Initial implementation of a TracerResolver for Jaeger #175

Initial implementation of a TracerResolver for Jaeger #175

Conversation

objectiser commented May 10, 2017

codecov-io commented May 10, 2017 • edited Loading

Codecov Report

mabn May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

objectiser commented May 12, 2017

yurishkuro commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 10, 2017 •

edited

Loading

mabn May 10, 2017 •

edited

Loading

pavolloffay May 10, 2017 •

edited

Loading