WIP: Auto-config for Extensions API tracer wrapper, metrics (with promethe… by objectiser · Pull Request #15 · opentracing-contrib/java-spring-cloud

objectiser · 2017-07-28T10:22:59Z

…us metrics reporter) and tracer resolver.

Prototype to investigate how we can add metrics (with prometheus reporter) support to an OpenTracing instrumented SB app with zero or minimal additional configuration.

This branch shows an updated accountmgr service that now only requires the maven dependencies, and additional metrics labels defined as beans (although these could be defined in a shared module to be completely outside scope of app):
https://github.com/objectiser/opentracing-prometheus-example/tree/autoconfig/simple/accountmgr

Currently contains a number of changes that could be separated out into their own PRs:

auto config for TracerResolver - so if tracer impl supports the resolver, then the app does not need to do anything regarding obtaining the tracer
extensions API tracer - only used to decorate the actual tracer if atleast one TracerObserver is found
metrics tracer observer
prometheus support - making the metrics reporter available (with any additional metric labels configured with the app) and if necessary establishing the servlet for the metrics exporter endpoint

…us metrics reporter) and tracer resolver.

malafeev · 2017-07-28T13:01:51Z

+@Configuration
+public class TracerBeanPostProcessor implements BeanPostProcessor {
+
+    private static final Logger log = Logger.getLogger(TracerBeanPostProcessor.class.getName());


why not slf4j logger?

All logging frameworks provide a logback for JUL, and JUL is the only logging framework that doesn't require an external dependency. So, it's usually a better choice for a library.

By the way: my point is moot if slf4j is already a dependency (I'm not sure that's the case)

I think we should use what is default/preferred in spring boot, and I guess it's not JUL

https://docs.spring.io/spring-boot/docs/current/reference/html/boot-features-logging.html
commons logging - so could switch.

jpkrohling

LGTM, although I can't judge on the Spring semantics.

jpkrohling · 2017-07-28T14:45:48Z

+    private String metricsName;
+
+    @Autowired(required=false)
+    private Set<MetricLabel> metricLabels;


Where is this coming from? Spring I guess, but what happens when required = false?

Yes - the app provides the MetricLabel beans. If none are provided, then metricLabels is null.

jpkrohling · 2017-07-28T14:46:55Z

+import org.springframework.context.annotation.Configuration;
+
+@Configuration
+@ConditionalOnClass(value = {io.prometheus.client.exporter.MetricsServlet.class,


I think it would be more readable if the classes are imported.

Need to check - but examples I have seen did not import the class, possibly because it may affect class loading of this class?

Just found some examples that use the annotation on imported classes, so I guess it isn't a problem - so will change.

jpkrohling · 2017-07-28T14:52:00Z

+
+        tracerObserver.onStart(spanData).onFinish(spanData, System.currentTimeMillis());
+
+        Mockito.verify(metricsReporter).reportSpan(spanData);


It's been a while since I last used mockito, so, I'm missing something here... You are asserting that reportSpan has been called and that the mocked spanData was used as parameter, but what actually called the reporter?

Essentially the resolved tracerObserver is a metrics observer configured with the mock metricsReporter - so when the onFinish is called with the mock spanData it gets passed through to the metrics reporter via the metrics observer.

jpkrohling · 2017-07-28T14:52:28Z

+
+    @Bean
+    public io.opentracing.Tracer tracer() {
+        return io.opentracing.contrib.tracerresolver.TracerResolver.resolveTracer();


Can this be imported?

Same reason as above - depends on whether this affects the ConditionalOnClass annotation.

…ass annotations

pavolloffay · 2017-07-31T07:45:48Z

auto config for TracerResolver - so if tracer impl supports the resolver, then the app does not need to do anything regarding obtaining the tracer

Could we make changes in TracerAutoConfiguration in spring-web module to get the tracer from GlobalTracer? If that works I think we can avoid tracer resolver bindings in this repo.

malafeev · 2017-07-31T07:51:35Z

@Bean 
public Tracer tracer() {
  return ...
}

couldn't work to avoid tracer resolver and GlobalTracer ?

objectiser · 2017-07-31T08:42:11Z

There are two issues issue - resolving the Tracer, and registering it with the GlobalTracer.

Dealing with GlobalTracer first - this will be necessary (I believe) as some of the framework instrumentations will not be spring based, so would not inject the tracer, but instead obtain it from GlobalTracer - so whatever tracer resolution mechanism is used, we need to make sure it is registered - which is not currently dealt with by this PR.

Now, in terms of resolving the Tracer, there are two options - either the application takes responsibility or we use the TracerResolver. The aim of the tracerresolver code in this PR is to only use the TracerResolver if it is in the classpath - and therefore the app developer has decided to use that approach.

Not saying that the current approach is ideal - it needs more work to understand how best to deal with resolving the Tracer bean - but I think it is important to support the mechanism as an option available to users.

objectiser · 2017-07-31T08:45:27Z

As a suggestion for dealing with registering with GlobalTracer - perhaps we just need a config that injects the Tracer and uses it to register on the GlobalTracer - so then no other code needs to worry about the registration? It will just use whatever Tracer has been resolved (through whatever mechanism) and it injected.

pavolloffay · 2017-07-31T09:28:56Z

How does the TracerResolver work? IIRC it registers the resolved tracer to GlobalTracer and the app obtains the tracer from it.

As I propose we can simply change the current TracerAutoConfiguration for fallback to GlobalTracer if there is no Tracer bean available in the context. Or does this PR has other benefits?

objectiser · 2017-07-31T09:42:42Z

Let me split out the tracer resolver part into a separate PR, as it needs to be discussed separately.

pavolloffay · 2017-08-01T11:27:22Z

+
+    @Override
+    public void contextInitialized(javax.servlet.ServletContextEvent sce) {
+        sce.getServletContext().setAttribute(TracingFilter.SKIP_PATTERN, Pattern.compile(metricsPath));


This skip pattern param is used only when the filter is registered via web.xml. In spring-web it is registered directly.

This wasn't intended as a real solution, hence starting discussion here opentracing-contrib/java-spring-web#28.

pavolloffay · 2017-08-01T11:27:43Z

+import io.prometheus.client.exporter.MetricsServlet;
+
+@Configuration
+@ConditionalOnClass(value = {MetricsServlet.class, TracingFilter.class})


I think this should not depend on TracingFilter

pavolloffay · 2017-08-01T11:38:34Z

I have been thinking about this for a while and we should define boundaries of this project. The purpose of this repository is to instrument spring cloud related frameworks.

This tracer/prometheus metrics integration is not spring/cloud related. Btw there is also spring boot metrics project https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-metrics.html and https://github.com/micrometer-metrics/micrometer (not sure how they overlap).

So far tracer instance has been obtained from spring context which is a standard way in spring I am not sure if tracer resolver autoconfig adds a benefit here. If users what to use it they can simple drop traver resolver dependency on classpath and write:

@Bean
public io.opentracing.tracer tracer() {
   return TracerResolver.resolveTracer();
}

and it is not necessary to have auto-config and starter artefact for it.

objectiser · 2017-08-01T11:49:56Z

I would view its scope slightly differently - I believe it is about making OpenTracing based instrumentation as easy as possible to consume within spring boot/cloud.

The metrics references you mentioned do not provide the same level of application metric granularity as java-metrics instrumentation based on OpenTracing.

Regarding TracerResolver autoconfig (which is now in a different PR) - the Tracer needs to get into the spring context somehow, what you are suggesting is that this must be done by the application developer, while I am suggesting that having the option to include it via auto-config may mean the app developer doesn't have to change any code. Isn't it better to give the developer a choice?

objectiser · 2017-08-01T12:30:41Z

As I am away for the rest of this week, what I would propose is that if no one has expressed an interest in auto-config for metrics being part of this repo by start of next week, then I'll move it to the java-metrics repo. Same for tracer resolver auto-config.

cc @malafeev @tedsuo @alesj

pavolloffay · 2017-08-01T12:34:16Z

what you are suggesting is that this must be done by the application developer, while I am suggesting that having the option to include it via auto-config may mean the app developer doesn't have to change any code. Isn't it better to give the developer a choice?

It's true, I would vote for a little explicit code rather than maintaining (in my opinion unnecessary) wrapper around trace resolver.

So if you want to integrate tracing you have to include certain dependencies in your class path. In other words change project source code e.g (pom, gradle). This would be all if we supported trace resolver starter. However, what I propose is to add one self-contained class:

@Configuration
public class TracingConfiguration {
   @Bean
   public io.opentracing.tracer tracer() {
      return TracerResolver.resolveTracer();
   }
}

You don't have to reference this class from anywhere. If you want to remove tracing you just remove it. I think it's better to stick with spring conventions and keep the configuration concise.

objectiser · 2017-08-08T14:20:17Z

Closing, moving autoconfigure code to:
opentracing-contrib/java-api-extensions#11
opentracing-contrib/java-metrics#16

Auto-config for Extensions API tracer wrapper, metrics (with promethe…

5bd652f

…us metrics reporter) and tracer resolver.

objectiser requested review from jpkrohling and pavolloffay July 28, 2017 10:23

malafeev reviewed Jul 28, 2017

View reviewed changes

jpkrohling reviewed Jul 28, 2017

View reviewed changes

Switch to commons-logging, use imports for classes in ConditionalOnCl…

0483eb1

…ass annotations

objectiser mentioned this pull request Jul 31, 2017

Add auto-config for the tracer resolver #16

Closed

Moved tracerresolver auto-config to a separate PR

95f44eb

pavolloffay reviewed Aug 1, 2017

View reviewed changes

This was referenced Aug 8, 2017

Provide spring autoconfiguration for the API extensions tracer opentracing-contrib/java-api-extensions#11

Merged

Add spring autoconfiguration for metrics and prometheus reporter opentracing-contrib/java-metrics#16

Merged

objectiser closed this Aug 8, 2017


		tracerObserver.onStart(spanData).onFinish(spanData, System.currentTimeMillis());

		Mockito.verify(metricsReporter).reportSpan(spanData);

Uh oh!

Conversation

objectiser commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpkrohling Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpkrohling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavolloffay commented Jul 31, 2017

Uh oh!

malafeev commented Jul 31, 2017

Uh oh!

objectiser commented Jul 31, 2017

Uh oh!

objectiser commented Jul 31, 2017

Uh oh!

pavolloffay commented Jul 31, 2017

Uh oh!

objectiser commented Jul 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavolloffay commented Aug 1, 2017

Uh oh!

objectiser commented Aug 1, 2017

Uh oh!

objectiser commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavolloffay commented Aug 1, 2017

Uh oh!

objectiser commented Aug 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jpkrohling Jul 28, 2017 •

edited

Loading

objectiser commented Aug 1, 2017 •

edited

Loading