Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename opentracing-spring-web-handler-interceptor to something more appropriate #8

Closed
pavolloffay opened this issue Mar 27, 2017 · 7 comments

Comments

@pavolloffay
Copy link
Collaborator

All the instrumentations are using org.springframework#spring-webmvc (spring web and http clients) and instrumentation artifactId is opentracing-spring-web-handler-interceptor and auto-configuration opentracing-spring-web-autoconfigure.

Should we change opentracing-spring-web-handler-interceptor to something like opentracing-spring-web/opentracing-spring-webmvc as it includes all instrumentations, both client and server side?

@pavolloffay
Copy link
Collaborator Author

I think we have these options:

  1. keep only opentracing-spring-web-handler-interceptor
  2. rename opentracing-spring-web-handler-interceptor to opentracing-spring-web
  3. introduce opentracing-spring-web-resttemplate

opentracing-spring-web-handler-interceptor uses spring-webmvc which internally uses spring-web.
Artifact with restTemplate instrumentation could use only spring-web which migh be better for some users which would like to use only rest template without the whole MVC stack.

@objectiser
Copy link
Contributor

I think it may be better to consolidate all spring (-web) related framework integration into a single repo. Otherwise there is potentially alot of fragmentation for essentially the same technology stack.

@pavolloffay
Copy link
Collaborator Author

I think it may be better to consolidate all spring (-web) related framework integration into a single repo. Otherwise there is potentially alot of fragmentation for essentially the same technology stack.

@objectiser it's not about another repo but about a separate artifact for client instrumentation. Client instrumentation would depend only on spring-web which contains the most important interfaces not the whole MVC stack.

Server side instrumentation depends on spring-webmvc which depends on spring-web. The reason to introduce new artifact is for users who would like to use RestTemplate without spring MVC/Boot stack.

e.g. there is also RestTemplate for android https://github.com/spring-projects/spring-android but it seems it does not depend on core spring-web so this should be fine.

Currently a combination of client and rest template into one instrumentation artifact seems ok, however if they separate these to we will have to create new artifact for a client side instrumentation.

Some useful links:
RestTemplate spring-web: https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/client/RestTemplate.java
ClientHttpRequestInterceptor spring-web: https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/http/client/ClientHttpRequestInterceptor.java
HandlerInterceptorAdapter: spring-webmvc: https://github.com/spring-projects/spring-framework/blob/master/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/HandlerInterceptorAdapter.java

@objectiser
Copy link
Contributor

@pavolloffay Sorry misunderstood - but same applies to artifact - best to have a single artifact, so would go with option 2.

Is there a particular reason autoconfigure is a separate artifact?

@pavolloffay
Copy link
Collaborator Author

Yes, autoconfigure does not work in plain MVC.

@pavolloffay
Copy link
Collaborator Author

I would like to keep instrumentations units as small as possible and then combine via something like autoconfigure. We might add more auto-configurations for even different frameworks - like in spring-cloud-sleuth.

@pavolloffay
Copy link
Collaborator Author

Closing, implemented in #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants